From 028eb6a66a3282c2693448640804ed0e4a23e256 Mon Sep 17 00:00:00 2001 From: Radoslav Karaivanov Date: Wed, 12 Aug 2026 09:03:41 +0300 Subject: [PATCH 1/3] fix(equal): track the pair being compared, not the two objects --- src/equal.ts | 138 ++++++++++++++++++++++++-------------------- tests/equal.spec.ts | 43 ++++++++++++++ 2 files changed, 117 insertions(+), 64 deletions(-) diff --git a/src/equal.ts b/src/equal.ts index 54492f3..b16ccc6 100644 --- a/src/equal.ts +++ b/src/equal.ts @@ -1,87 +1,97 @@ -export function equal(a: unknown, b: T, visited = new WeakSet()): boolean { +/** Pairs currently being compared, so a cycle terminates without swallowing real differences. */ +type Visited = WeakMap>; + +export function equal(a: unknown, b: T, visited: Visited = new WeakMap()): boolean { // Early return if (Object.is(a, b)) return true; - if (isObject(a) && isObject(b)) { - if (a.constructor !== b.constructor) return false; + if (!isObject(a) || !isObject(b)) return false; + if (a.constructor !== b.constructor) return false; - // Circular references - if (visited.has(a) && visited.has(b)) { - return true; - } + // Circular references. This tracks the *pair*, not the two objects separately: the Map and Set + // comparisons below probe candidates they expect to fail, and marking those objects on their own + // would make a later comparison of the same pair short circuit to `true`. + let pending = visited.get(a); + if (pending?.has(b)) return true; + + if (!pending) { + pending = new WeakSet(); + visited.set(a, pending); + } + pending.add(b); + + try { + return compare(a, b, visited); + } finally { + // Always release the pair, including on the early returns in `compare`. + pending.delete(b); + } +} - visited.add(a); - visited.add(b); - - // RegExp - if (isRegExp(a) && isRegExp(b)) return a.source === b.source && a.flags === b.flags; - - // Maps - if (a instanceof Map && b instanceof Map) { - if (a.size !== b.size) return false; - for (const [keyA, valueA] of a.entries()) { - let found = false; - for (const [keyB, valueB] of b.entries()) { - if (equal(keyA, keyB, visited) && equal(valueA, valueB, visited)) { - found = true; - break; - } +function compare(a: object, b: object, visited: Visited): boolean { + // RegExp + if (isRegExp(a) && isRegExp(b)) return a.source === b.source && a.flags === b.flags; + + // Maps + if (a instanceof Map && b instanceof Map) { + if (a.size !== b.size) return false; + for (const [keyA, valueA] of a.entries()) { + let found = false; + for (const [keyB, valueB] of b.entries()) { + if (equal(keyA, keyB, visited) && equal(valueA, valueB, visited)) { + found = true; + break; } - if (!found) return false; } - return true; + if (!found) return false; } + return true; + } - // Sets - if (a instanceof Set && b instanceof Set) { - if (a.size !== b.size) return false; - for (const valueA of a) { - let found = false; - for (const valueB of b) { - if (equal(valueA, valueB, visited)) { - found = true; - break; - } + // Sets + if (a instanceof Set && b instanceof Set) { + if (a.size !== b.size) return false; + for (const valueA of a) { + let found = false; + for (const valueB of b) { + if (equal(valueA, valueB, visited)) { + found = true; + break; } - if (!found) return false; } - return true; + if (!found) return false; } + return true; + } - // Arrays - if (Array.isArray(a) && Array.isArray(b)) { - const length = a.length; - if (length !== b.length) return false; - for (let i = 0; i < length; i++) { - if (!equal(a[i], b[i], visited)) return false; - } - return true; + // Arrays + if (Array.isArray(a) && Array.isArray(b)) { + const length = a.length; + if (length !== b.length) return false; + for (let i = 0; i < length; i++) { + if (!equal(a[i], b[i], visited)) return false; } + return true; + } - // toPrimitive - if (a.valueOf !== Object.prototype.valueOf) return a.valueOf() === b.valueOf(); - // Strings based - if (a.toString !== Object.prototype.toString) return a.toString() === b.toString(); - - const aKeys = Object.keys(a); - const bKeys = Object.keys(b); - if (aKeys.length !== bKeys.length) return false; - - for (const key of aKeys) { - if (!Object.hasOwn(b, key)) return false; - } + // toPrimitive + if (a.valueOf !== Object.prototype.valueOf) return a.valueOf() === b.valueOf(); + // Strings based + if (a.toString !== Object.prototype.toString) return a.toString() === b.toString(); - for (const key of aKeys) { - if (!equal(a[key as keyof typeof a], b[key as keyof typeof b], visited)) return false; - } + const aKeys = Object.keys(a); + const bKeys = Object.keys(b); + if (aKeys.length !== bKeys.length) return false; - visited.delete(a); - visited.delete(b); + for (const key of aKeys) { + if (!Object.hasOwn(b, key)) return false; + } - return true; + for (const key of aKeys) { + if (!equal(a[key as keyof typeof a], b[key as keyof typeof b], visited)) return false; } - return false; + return true; } function isObject(value: unknown): value is object { diff --git a/tests/equal.spec.ts b/tests/equal.spec.ts index 6157515..8d4f1fa 100644 --- a/tests/equal.spec.ts +++ b/tests/equal.spec.ts @@ -337,4 +337,47 @@ describe('equal', () => { const set2 = new Set([]); expect(equal(set1, set2)).toBe(false); }); + it('should not let failed Set candidate pairs poison later comparisons', () => { + const a = new Set([{ x: 1 }, { x: 2 }]); + const b = new Set([{ x: 2 }, { x: 1 }]); + expect(equal(a, b)).toBe(true); + + // `{ k: 'b' }` has no counterpart, so these Sets differ. Getting there walks candidate pairs + // that fail, and those failures must not be remembered as matches. + const left = new Set([{ k: 'z' }, { k: 'b' }, { k: 'x' }]); + const right = new Set([{ k: 'x' }, { k: 'y' }, { k: 'z' }]); + expect(equal(left, right)).toBe(false); + }); + + it('should not let failed Map candidate pairs poison later comparisons', () => { + const a = new Map([ + ['a', { v: 1 }], + ['b', { v: 2 }], + ]); + const b = new Map([ + ['b', { v: 2 }], + ['a', { v: 1 }], + ]); + expect(equal(a, b)).toBe(true); + + const mismatched = new Map([ + ['a', { v: 1 }], + ['b', { v: 3 }], + ]); + expect(equal(a, mismatched)).toBe(false); + }); + + it('should still terminate on circular references', () => { + const a: Record = { name: 'a' }; + const b: Record = { name: 'a' }; + a.self = a; + b.self = b; + expect(equal(a, b)).toBe(true); + + const c: Record = { name: 'c' }; + const d: Record = { name: 'different' }; + c.self = c; + d.self = d; + expect(equal(c, d)).toBe(false); + }); }); From 48069f5c073ab30204e3ff3d54058f51246b9149 Mon Sep 17 00:00:00 2001 From: Radoslav Karaivanov Date: Wed, 12 Aug 2026 09:05:08 +0300 Subject: [PATCH 2/3] fix(render-props): keep render prop state alive across renders The render prop machinery kept its state in per-render locals while the element held on to the patched templates indefinitely, so the two drifted apart the moment the component re-rendered. Three defects came out of that: * Templates rendered against a stale closure. The patched function handed to the element was cached on first sight - but so was the callback it invoked, so a template closing over React state kept rendering the value it saw on the very first render. * An `undefined` render prop spun the render loop. A conditional prop (`headerTemplate={enabled ? tpl : undefined}`) was still treated as a template: it installed an empty portal and recreated the patched function on every render. Each new identity re-rendered the element, which re-requested the template, at roughly 97 cycles a second. The churn also blanked sibling templates on the same component. * Portal updates were lost. The map of active slots was cloned per render, so a request arriving from the element mutated its own copy and a later render overwrote it. That state now lives on a `TemplateBridge` owned by the component instance - patched templates keyed by prop path, current callbacks keyed by renderer name, the slots, and the nested prop containers. Callbacks are refreshed on every render while the patched templates keep their identity, so the element sees a stable prop and the template always runs the current closure. Portals are built when the element requests a slot, and rebuilt only when the render prop's identity changes. Pruning is driven by a single predicate - is there still a function at this prop path? - which covers both a removed prop and one that turned `undefined`. It also drops the matching slots: the directive's remove request travels through a `WeakRef` and may never arrive once the patched template is gone, leaving the portal to linger for the component's lifetime. Nested prop containers reuse their previous object while shallow-equal, so a config object carrying a template stops changing identity every render. The rest of the file follows the same split. Ref forwarding and the Angular re-parenting effect move into `useForwardedRef` and `useReparenting`, leaving `createComponent` with registration and a nine-line component. In `render-props.ts`: `_renderNode` could return `undefined` behind an `as Element` cast; requests now go through a helper that bails when there is no node. `render()` is typed as `RendererCallback` so `any` stops leaking into `update()`. `_state.previous` resets on every disconnect, not just some. Adds regression tests for the three defects, each confirmed to fail against the previous code. --- src/react-props.tsx | 483 ++++++++++++++++++++--------- src/render-props.ts | 28 +- tests/grid-lite/Templates.spec.tsx | 50 +++ tests/grid-lite/Templates.tsx | 68 ++++ 4 files changed, 475 insertions(+), 154 deletions(-) create mode 100644 tests/grid-lite/Templates.spec.tsx create mode 100644 tests/grid-lite/Templates.tsx diff --git a/src/react-props.tsx b/src/react-props.tsx index c457c9f..8f4510d 100644 --- a/src/react-props.tsx +++ b/src/react-props.tsx @@ -36,12 +36,21 @@ type ComponentProps = Omit< EventListeners & ElementProps; -/** Mapped type to update the render props callback return type */ -type WithJsxRenderProps = { +/** + * Mapped type to update the render props callback return type. + * + * A renderer entry is either a name (a template directly on the component) or a nested map of + * them (a template on a config object, as `igc-chat` declares its renderers), so it recurses. + */ +type WithJsxRenderProps = { [K in keyof T]: K extends keyof R - ? NonNullable extends (...args: infer Args) => unknown - ? (...args: WithDataContext) => React.ReactNode - : T[K] + ? R[K] extends string + ? NonNullable extends (...args: infer Args) => unknown + ? ((...args: WithDataContext) => React.ReactNode) | Extract + : T[K] + : R[K] extends Renderers + ? WithJsxRenderProps, R[K]> | Extract + : T[K] : T[K]; }; @@ -55,14 +64,295 @@ export type ReactWebComponent< PropsWithoutRef, R>> & React.RefAttributes >; +/** A map of prop names to renderer names, nested to mirror the shape of the props themselves. */ type Renderers = Record; +/** The React module, which is injected rather than imported so Preact can be swapped in. */ +type ReactModule = typeof React; + +type Props = Record; +type RenderProp = (data: unknown) => React.ReactNode; + +/** A patched template handed to the element, and the renderer it stands in for. */ +type PatchedTemplate = { + patched: (ctx: unknown) => unknown; + rendererName: string; +}; + +/** A slot the element has asked us to fill, and the portal currently filling it. */ +type PortalSlot = { + name: string; + data: unknown; + node: Element; + callback: RenderProp | undefined; + portal: React.ReactPortal; +}; + interface WrapperOptions extends Options { renderProps?: R; moveBackOnDelete?: boolean; } +/** + * Owns the render prop machinery of a single component instance. + * + * All of this state has to outlive the render that created it: the element holds on to the patched + * templates indefinitely and can invoke them whenever it likes, so what they read and write belongs + * to the instance rather than to a render. + */ +class TemplateBridge { + /** Patched templates handed to the element, keyed by their full prop path. */ + private readonly _templates = new Map(); + + /** The *current* render prop callbacks, keyed by renderer name. */ + private readonly _callbacks = new Map(); + + /** Slots the element has asked us to fill, keyed by slot name. */ + private readonly _slots = new Map(); + + /** Nested prop containers, kept around so their identity is stable while their contents are. */ + private readonly _containers = new Map(); + + private readonly _renderers: Renderers; + private readonly _notify: () => void; + + constructor(renderers: Renderers, notify: () => void) { + this._renderers = renderers; + this._notify = notify; + } + + /** + * Turns the component props into the props handed to the element, swapping every render prop for + * a patched template of stable identity. + */ + public resolve(props: Props): Props { + const elementProps: Props = {}; + + this._prune(props); + this._collect(props, this._renderers, elementProps); + this._refresh(); + + return elementProps; + } + + /** The portals currently filling the element's slots. */ + public *portals(): Generator { + for (const { portal } of this._slots.values()) { + yield portal; + } + } + + /** + * Fills or clears a slot at the element's request. Bound once per instance, since the patched + * templates hold on to it for as long as the element does. + */ + private readonly _request = (req: RendererRequest): void => { + if (req.data === REQUEST_REMOVE) { + this._slots.delete(req.slotName); + } else { + const callback = this._callbacks.get(req.name); + + this._slots.set(req.slotName, { + name: req.name, + data: req.data, + node: req.node, + callback, + portal: createPortal(callback?.(req.data), req.node, req.slotName), + }); + } + + this._notify(); + }; + + /** + * Drops patched templates whose render prop is gone - either the prop was removed, or it is no + * longer a function. Both have to be pruned: leaving one behind hands the element a stale + * template, while recreating one on every render feeds it a new identity and spins the loop. + */ + private _prune(props: Props): void { + for (const [path, { rendererName }] of this._templates) { + if (typeof getAtPath(props, path) === 'function') { + continue; + } + + this._templates.delete(path); + this._callbacks.delete(rendererName); + + // Drop the slots as well. The directive does emit a remove request when it disconnects, but + // it reaches for the callback through a `WeakRef` - once the patched template above is gone + // that request may never arrive, and the portal would linger for the component's lifetime. + for (const [slotName, slot] of this._slots) { + if (slot.name === rendererName) { + this._slots.delete(slotName); + } + } + } + } + + /** Copies the props over, swapping render props for templates and recursing into config objects. */ + private _collect(props: Props, renderers: Renderers, out: Props, prefix = ''): void { + for (const prop in props) { + const path = prefix ? `${prefix}.${prop}` : prop; + const renderer = renderers[prop]; + const value = props[prop]; + + if (typeof renderer === 'string') { + out[prop] = this._template(path, renderer, value); + } else if (isRecord(renderer) && isRecord(value)) { + const nested: Props = {}; + + this._collect(value, renderer, nested, path); + out[prop] = this._container(path, nested); + } else { + out[prop] = value; + } + } + } + + /** + * The patched template standing in for a render prop, created on first sight. + * + * Only a function is a template. Anything else - most often `undefined` from a conditional prop - + * has to reach the element untouched so it can fall back to its own default rendering. + */ + private _template(path: string, name: string, value: unknown): unknown { + if (typeof value !== 'function') { + return value; + } + + // Refreshed on every render. The patched template is cached so the element sees a stable prop, + // but the callback it invokes must always be the current one, or the template renders against + // a stale closure. + this._callbacks.set(name, value as RenderProp); + + let template = this._templates.get(path); + + if (!template) { + template = { patched: createPatched(this._request, name), rendererName: name }; + this._templates.set(path, template); + } + + return template.patched; + } + + /** Reuses the previous container while its contents are unchanged, to keep its identity stable. */ + private _container(path: string, next: Props): Props { + const previous = this._containers.get(path); + + if (previous && shallowEqual(previous, next)) { + return previous; + } + + this._containers.set(path, next); + return next; + } + + /** + * A portal is built when the element requests its slot and reused as-is afterwards - handing + * React a fresh portal on every render would re-commit the template into the element, which + * re-renders the element, which requests the template again. + * + * It does have to be rebuilt when the render prop itself changes, though. Templates commonly + * close over state, and the element has no reason to re-request one just because the React tree + * above it re-rendered. + */ + private _refresh(): void { + for (const [slotName, slot] of this._slots) { + const callback = this._callbacks.get(slot.name); + + if (slot.callback !== callback) { + slot.callback = callback; + slot.portal = createPortal(callback?.(slot.data), slot.node, slotName); + } + } + } +} + +/** Creates the render prop state of this component instance and keeps it for its lifetime. */ +function useTemplateBridge(react: ReactModule, renderers: Renderers): TemplateBridge { + const [, forceUpdate] = react.useReducer(increment, 0); + const bridge = react.useRef(null); + + // `forceUpdate` is stable for the lifetime of the component, so capturing the first one is safe. + bridge.current ??= new TemplateBridge(renderers, forceUpdate); + + return bridge.current; +} + +/** Forwards the ref while keeping a local handle on the element for the hooks that need one. */ +function useForwardedRef( + react: ReactModule, + ref: React.ForwardedRef, +): readonly [React.RefObject, (node: I) => void] { + const elementRef = react.useRef(null); + + const setRef = react.useCallback( + (node: I) => { + elementRef.current = node; + + if (typeof ref === 'function') { + ref(node); + } else if (ref !== null) { + ref.current = node; + } + }, + [ref], + ); + + return [elementRef, setRef]; +} + +/** + * Handles element re-parenting for the Angular integration, where Angular Elements moves a + * projected element away from the parent React knows about. + */ +function useReparenting( + react: ReactModule, + enabled: boolean | undefined, + elementRef: React.RefObject, +): void { + const projectionParent = react.useRef | null>(null); + + // https://react.dev/learn/reusing-logic-with-custom-hooks#keep-your-custom-hooks-focused-on-concrete-high-level-use-cases + // Runs once after first render. + react.useLayoutEffect(() => { + if (!enabled) { + return; + } + + // already too late to save elementRef.current?.parentElement, rely on Elements + // secondary run (likely dev strict mode), move back to projection: + const prevParent = projectionParent.current?.deref(); + + if (prevParent && elementRef.current && prevParent !== elementRef.current.parentElement) { + prevParent.appendChild(elementRef.current); + } + projectionParent.current = null; + + return () => { + // cleanup **before** component is removed from the DOM + const element = elementRef.current; + + if (!element) { + return; + } + + const creationParent = ( + element as I & { ngElementStrategy?: { parentElement?: WeakRef } } + ).ngElementStrategy?.parentElement?.deref(); + + if (creationParent && creationParent !== element.parentElement) { + // move back to original parent + if (element.parentElement) { + projectionParent.current = new WeakRef(element.parentElement); + } + creationParent.appendChild(element); + } + }; + }, [enabled, elementRef]); +} + export const createComponent = < I extends HTMLElement, E extends EventNames = {}, @@ -81,152 +371,37 @@ export const createComponent = < (elementClass as { register: () => void }).register(); } - if (!renderProps && !moveBackOnDelete) { - // When R is empty (no renderProps), the component types are equivalent at runtime - return _createComponent({ - react: React, - tagName, - elementClass, - events, - displayName, - }) as unknown as ReactWebComponent; - } - - type Props = ComponentProps; - - const safeEvents = events ?? ({} as E); const component = _createComponent({ react: React, tagName, elementClass, - events: safeEvents, + events, displayName, }); - type PropsWithRenderProps = WithJsxRenderProps; + if (!renderProps && !moveBackOnDelete) { + // When R is empty (no renderProps), the component types are equivalent at runtime + return component as unknown as ReactWebComponent; + } - return React.forwardRef((props, ref) => { - const listeners = React.useRef(new Map()); - const elementRef = React.useRef(null); - const projectionParent = React.useRef | null>(null); - const [renderers, setRenderers] = React.useState(new Map()); - const outProps: Record = {}; - const portals: Record React.ReactNode> = {}; - - // https://react.dev/learn/reusing-logic-with-custom-hooks#keep-your-custom-hooks-focused-on-concrete-high-level-use-cases - // Runs once after first render to handle element re-parenting for Angular integration. - React.useLayoutEffect(() => { - if (!moveBackOnDelete) return; - - // already too late to save elementRef.current?.parentElement, rely on Elements - // secondary run (likely dev strict mode), move back to projection: - const prevParent = projectionParent.current?.deref(); - if (prevParent && elementRef.current && prevParent !== elementRef.current.parentElement) { - prevParent.appendChild(elementRef.current); - projectionParent.current = null; - } - return () => { - // cleanup **before** component is removed from the DOM - const element = elementRef.current; - if (!element) return; - - const creationParent = ( - element as I & { ngElementStrategy?: { parentElement?: WeakRef } } - ).ngElementStrategy?.parentElement?.deref(); - if (creationParent && creationParent !== element.parentElement) { - // move back to original parent - if (element.parentElement) { - projectionParent.current = new WeakRef(element.parentElement); - } - creationParent.appendChild(element); - } - }; - }, [moveBackOnDelete]); - - // Don't wrap in an `useCallback` hook since there is no mechanism in React to dispose of the cached function(s), - // potentially leading to a memory leak/higher memory usage for heavily templated component instances. - const renderFunc = (req: RendererRequest) => { - if (req.data === REQUEST_REMOVE) { - renderers.delete(req.slotName); - } else { - renderers.set( - req.slotName, - createPortal(portals[req.name]?.(req.data), req.node, req.slotName), - ); - } - setRenderers(() => new Map(renderers)); - }; + const renderers: Renderers = renderProps ?? {}; - const processProps = ( - propMap: Record, - propDefinitions: Record, - outProps: Record, - prefix = '', - ) => { - for (const prop in propMap) { - const fullPropName = prefix ? `${prefix}.${prop}` : prop; - const rendererName = propDefinitions?.[prop]; - - if (rendererName !== undefined && typeof rendererName === 'string') { - if (listeners.current.has(fullPropName)) { - outProps[prop] = listeners.current.get(fullPropName); - } else { - portals[rendererName] = propMap[prop] as (data: unknown) => React.ReactNode; - const patched = createPatched(renderFunc, rendererName); - outProps[prop] = patched; - listeners.current.set(fullPropName, patched); - } - } else if ( - typeof propMap[prop] === 'object' && - propMap[prop] !== null && - propDefinitions?.[prop] && - typeof propDefinitions[prop] === 'object' - ) { - outProps[prop] = {}; - processProps( - propMap[prop] as Record, - propDefinitions?.[prop] as Record, - outProps[prop] as Record, - fullPropName, - ); - } else { - outProps[prop] = propMap[prop]; - } - } - }; + type PropsWithRenderProps = WithJsxRenderProps, R>; - for (const key of listeners.current.keys()) { - if (!hasNestedProperty(props, key)) { - listeners.current.delete(key); - } - } + return React.forwardRef((props, ref) => { + const bridge = useTemplateBridge(React, renderers); + const [elementRef, setRef] = useForwardedRef(React, ref); + useReparenting(React, moveBackOnDelete, elementRef); - processProps(props, renderProps ?? {}, outProps); + const elementProps = bridge.resolve(props as Props); + const children = React.Children.toArray((props as { children?: React.ReactNode }).children); - const propsWithChildren = props as Props & { children?: React.ReactNode }; - if (listeners.current.size) { - Object.assign(outProps, { - children: [...React.Children.toArray(propsWithChildren.children), ...renderers.values()], - }); - } else { - Object.assign(outProps, { - children: React.Children.toArray(propsWithChildren.children), - }); - } + children.push(...bridge.portals()); + elementProps.children = children; return React.createElement(component, { - ...outProps, - ref: React.useCallback( - (node: I) => { - elementRef.current = node; - if (typeof ref === 'function') { - ref(node); - } else if (ref !== null) { - ref.current = node; - } - }, - [ref], - ), + ...elementProps, + ref: setRef, } as PropsWithoutRef> & React.RefAttributes); }); }; @@ -235,14 +410,34 @@ function createPatched(callback: (req: RendererRequest) => void, proper return (ctx: unknown) => html`${requestRenderer(callback, propertyName, ctx)}`; } -function hasNestedProperty(object: Record, path: string): boolean { - const parts = path.split('.'); +function increment(count: number): number { + return count + 1; +} + +function isRecord(value: unknown): value is Props { + return typeof value === 'object' && value !== null; +} + +function shallowEqual(a: Props, b: Props): boolean { + const keys = Object.keys(a); + + if (keys.length !== Object.keys(b).length) { + return false; + } + + return keys.every((key) => Object.is(a[key], b[key])); +} + +/** Resolves a dot delimited path, returning `undefined` if any segment is missing. */ +function getAtPath(object: Props, path: string): unknown { let current: unknown = object; - for (const part of parts) { - if (current === undefined || current === null || typeof current !== 'object') { - return false; + + for (const part of path.split('.')) { + if (!isRecord(current)) { + return undefined; } - current = (current as Record)[part]; + current = current[part]; } - return current !== undefined; + + return current; } diff --git a/src/render-props.ts b/src/render-props.ts index edfbb3c..725bf6a 100644 --- a/src/render-props.ts +++ b/src/render-props.ts @@ -12,7 +12,7 @@ import { getUUID } from './random-uuid.js'; export const REQUEST_REMOVE = Symbol('renderer-remove'); const NOT_SET = Symbol('not-set'); -type NgState = T & { $implicit: unknown }; +type NgState = T & { implicit: unknown }; type RendererState = { previous: T; current: T; @@ -49,8 +49,8 @@ class RequestRenderer extends AsyncDirective { private _state = { previous: NOT_SET, current: undefined } as RendererState; private _name!: string; - private get _renderNode(): Element { - return this._part?.deref()?.parentNode as Element; + private get _renderNode(): Element | undefined { + return this._part?.deref()?.parentNode as Element | undefined; } private _shouldUpdateNG(_data: NgState): boolean { @@ -68,7 +68,7 @@ class RequestRenderer extends AsyncDirective { private _shouldUpdate(): boolean { const data = this._state.current; - if (Reflect.has(data as NgState, 'implicit')) { + if (data !== null && typeof data === 'object' && Reflect.has(data as NgState, 'implicit')) { return this._shouldUpdateNG(data as NgState); } @@ -80,7 +80,15 @@ class RequestRenderer extends AsyncDirective { return true; } - public override render(_callback: any, _name: string, _data: T): symbol { + /** Dispatches a request for the current state, if there is somewhere to render it. */ + private _request(callback: RendererCallback, data: T | typeof REQUEST_REMOVE): void { + const node = this._renderNode; + if (!node) return; + + callback(createRequestData(this._name, data, node, this._key)); + } + + public override render(_callback: RendererCallback, _name: string, _data: T): symbol { return noChange; } @@ -94,7 +102,7 @@ class RequestRenderer extends AsyncDirective { this._part = new WeakRef(part); if (this.isConnected && callback && this._shouldUpdate()) { - callback(createRequestData(this._name, this._state.current, this._renderNode, this._key)); + this._request(callback, this._state.current); } return noChange; @@ -103,17 +111,17 @@ class RequestRenderer extends AsyncDirective { protected override reconnected(): void { const callback = this._callback?.deref(); if (callback && this._shouldUpdate()) { - callback(createRequestData(this._name, this._state.current, this._renderNode, this._key)); + this._request(callback, this._state.current); } } protected override disconnected(): void { const callback = this._callback?.deref(); if (callback) { - callback(createRequestData(this._name, REQUEST_REMOVE as T, this._renderNode, this._key)); - // drop prev, so a reconnect would behave like initial - this._state.previous = NOT_SET as T; + this._request(callback, REQUEST_REMOVE); } + // drop prev, so a reconnect would behave like initial + this._state.previous = NOT_SET as T; } } diff --git a/tests/grid-lite/Templates.spec.tsx b/tests/grid-lite/Templates.spec.tsx new file mode 100644 index 0000000..09eb960 --- /dev/null +++ b/tests/grid-lite/Templates.spec.tsx @@ -0,0 +1,50 @@ +import { expect, test } from 'vitest'; +import { page } from 'vitest/browser'; +import { render } from 'vitest-browser-react'; +import { OptionalTemplate, StatefulTemplate } from './Templates'; + +const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); + +test('render props observe updated React state', async () => { + render(); + + await expect.element(page.getByText('V:1/C:0', { exact: true })).toBeVisible(); + + await page.getByRole('button', { name: 'Increment' }).click(); + + // the template closes over `count` - it must re-render with the new value + await expect.element(page.getByText('V:1/C:1', { exact: true })).toBeVisible(); + await expect.element(page.getByText('V:2/C:1', { exact: true })).toBeVisible(); +}); + +test('a render prop passed as undefined leaves its siblings intact', async () => { + render(); + + await expect.element(page.getByText('V:1', { exact: true })).toBeVisible(); + await expect.element(page.getByText('V:2', { exact: true })).toBeVisible(); +}); + +test('a render prop passed as undefined does not spin the render loop', async () => { + render(); + + const cell = page.getByText('V:1', { exact: true }); + await expect.element(cell).toBeVisible(); + + // portals are torn down and recreated on every cycle, so a settled component + // keeps the same DOM node + const node = cell.element(); + await delay(300); + expect(cell.element()).toBe(node); +}); + +test('a render prop added on a later render does not evict the existing ones', async () => { + render(); + + await expect.element(page.getByText('V:1', { exact: true })).toBeVisible(); + + await page.getByRole('button', { name: 'Add header template' }).click(); + + await expect.element(page.getByText('H:id', { exact: true })).toBeVisible(); + await expect.element(page.getByText('V:1', { exact: true })).toBeVisible(); + await expect.element(page.getByText('V:2', { exact: true })).toBeVisible(); +}); diff --git a/tests/grid-lite/Templates.tsx b/tests/grid-lite/Templates.tsx new file mode 100644 index 0000000..d16d786 --- /dev/null +++ b/tests/grid-lite/Templates.tsx @@ -0,0 +1,68 @@ +import { useMemo, useState } from 'react'; +import { + type IgrCellContext, + IgrGridLite, + IgrGridLiteColumn, + type IgrHeaderContext, +} from '../../src/grid-lite'; +import '../../node_modules/igniteui-webcomponents/themes/light/bootstrap.css'; + +interface Person { + id: number; + name: string; +} + +const data: Person[] = [ + { id: 1, name: 'Alice' }, + { id: 2, name: 'Bob' }, +]; + +/** A cell template closing over React state. */ +export function StatefulTemplate() { + const [count, setCount] = useState(0); + const records = useMemo(() => data, []); + + const cellTemplate = (ctx: IgrCellContext) => ( + + V:{ctx.value}/C:{count} + + ); + + return ( + <> + + + + + + + ); +} + +/** A render prop that is conditionally `undefined`, next to one that is always set. */ +export function OptionalTemplate() { + const [withHeader, setWithHeader] = useState(false); + const records = useMemo(() => data, []); + + const cellTemplate = (ctx: IgrCellContext) => V:{ctx.value}; + const headerTemplate = (ctx: IgrHeaderContext) => H:{ctx.column.field}; + + return ( + <> + + + + + + + ); +} From 8a4be867a2290dd1a46dfdd31b024f91d0bb1799 Mon Sep 17 00:00:00 2001 From: Radoslav Karaivanov Date: Mon, 28 Sep 2026 09:01:27 +0300 Subject: [PATCH 3/3] fix: Fix render prop state across renders Render prop state lived in per-render locals, while the element kept its patched templates indefinitely. The two drifted apart on every re-render: * Stale closures. A template reading React state kept the callback it saw first. * `undefined` render props rendered blank instead of the element's default. * A render prop added later evicted the others: requests updated a stale copy of the slot map. * Nested config objects, like `igc-chat`'s `options`, changed identity every render, so the element re-rendered with them. A `TemplateBridge` per component instance now owns that state. Templates keep their identity and close over nothing but the renderer name. Element requests only record slots; render fills them from the current props. Async render props are resolved by the bridge, which keeps the previous content until the new one settles. Suspending on them would never settle, since inline render props yield a new promise per render. Rejections rethrow on render for error boundaries; the nearest `` no longer sees them. `WithJsxRenderProps` recurses into nested renderer maps and keeps optional render props optional. Also fixes: * `equal` let failed Set/Map probes mark objects as visited, so collections with different contents compared equal. It now tracks pairs, matches entries one to one, and handles null-prototype objects. * Templates invoked without a context threw: `withDataContext` proxied non-objects. --- src/backfills.ts | 7 + src/equal.ts | 126 ++++++----- src/is-object.ts | 3 + src/react-props.tsx | 350 +++++++++++------------------ src/render-props.ts | 107 ++++----- tests/backfills.spec.ts | 9 + tests/equal.spec.ts | 47 ++-- tests/grid-lite/Templates.spec.tsx | 58 +++-- tests/grid-lite/Templates.tsx | 85 +++++-- tests/web-components/Chat.spec.tsx | 12 +- tests/web-components/Chat.tsx | 24 ++ 11 files changed, 441 insertions(+), 387 deletions(-) create mode 100644 src/is-object.ts create mode 100644 tests/backfills.spec.ts diff --git a/src/backfills.ts b/src/backfills.ts index 34a545a..7b0a1e9 100644 --- a/src/backfills.ts +++ b/src/backfills.ts @@ -1,3 +1,5 @@ +import { isObject } from './is-object.js'; + const DATA_CONTEXT_PROP = 'dataContext'; let once = false; @@ -6,6 +8,11 @@ let once = false; * the `dataContext` prop returning the original data */ export function withDataContext(data: unknown) { + // Proxy throws on primitives. + if (!isObject(data)) { + return data; + } + return new Proxy(data as any, { get(target, prop, receiver) { if (prop === DATA_CONTEXT_PROP) { diff --git a/src/equal.ts b/src/equal.ts index b16ccc6..6503604 100644 --- a/src/equal.ts +++ b/src/equal.ts @@ -1,101 +1,115 @@ -/** Pairs currently being compared, so a cycle terminates without swallowing real differences. */ +import { isObject } from './is-object.js'; + type Visited = WeakMap>; -export function equal(a: unknown, b: T, visited: Visited = new WeakMap()): boolean { +export function equal(a: unknown, b: T, visited?: Visited): boolean { // Early return - if (Object.is(a, b)) return true; + if (Object.is(a, b)) { + return true; + } + + if (!isObject(a) || !isObject(b)) { + return false; + } - if (!isObject(a) || !isObject(b)) return false; - if (a.constructor !== b.constructor) return false; + if (a.constructor !== b.constructor) { + return false; + } - // Circular references. This tracks the *pair*, not the two objects separately: the Map and Set - // comparisons below probe candidates they expect to fail, and marking those objects on their own - // would make a later comparison of the same pair short circuit to `true`. - let pending = visited.get(a); - if (pending?.has(b)) return true; + // Track pairs, not objects: failed Set/Map probes must not count as visited. + const pairs: Visited = visited ?? new WeakMap(); + const pending = pairs.get(a) ?? new WeakSet(); - if (!pending) { - pending = new WeakSet(); - visited.set(a, pending); + if (pending.has(b)) { + return true; } - pending.add(b); + + pairs.set(a, pending.add(b)); try { - return compare(a, b, visited); + return compare(a, b, pairs); } finally { - // Always release the pair, including on the early returns in `compare`. pending.delete(b); } } function compare(a: object, b: object, visited: Visited): boolean { // RegExp - if (isRegExp(a) && isRegExp(b)) return a.source === b.source && a.flags === b.flags; - - // Maps - if (a instanceof Map && b instanceof Map) { - if (a.size !== b.size) return false; - for (const [keyA, valueA] of a.entries()) { - let found = false; - for (const [keyB, valueB] of b.entries()) { - if (equal(keyA, keyB, visited) && equal(valueA, valueB, visited)) { - found = true; - break; - } - } - if (!found) return false; - } - return true; + if (isRegExp(a) && isRegExp(b)) { + return a.source === b.source && a.flags === b.flags; } - // Sets - if (a instanceof Set && b instanceof Set) { - if (a.size !== b.size) return false; - for (const valueA of a) { - let found = false; - for (const valueB of b) { - if (equal(valueA, valueB, visited)) { - found = true; - break; - } - } - if (!found) return false; - } - return true; + // Map entries iterate as [key, value] arrays. + if ((a instanceof Map && b instanceof Map) || (a instanceof Set && b instanceof Set)) { + return a.size === b.size && matchOneToOne(a, b, visited); } // Arrays if (Array.isArray(a) && Array.isArray(b)) { const length = a.length; - if (length !== b.length) return false; + + if (length !== b.length) { + return false; + } + for (let i = 0; i < length; i++) { - if (!equal(a[i], b[i], visited)) return false; + if (!equal(a[i], b[i], visited)) { + return false; + } } return true; } - // toPrimitive - if (a.valueOf !== Object.prototype.valueOf) return a.valueOf() === b.valueOf(); + // toPrimitive; absent on null-prototype objects. + if (typeof a.valueOf === 'function' && a.valueOf !== Object.prototype.valueOf) { + return a.valueOf() === b.valueOf(); + } + // Strings based - if (a.toString !== Object.prototype.toString) return a.toString() === b.toString(); + if (typeof a.toString === 'function' && a.toString !== Object.prototype.toString) { + return a.toString() === b.toString(); + } const aKeys = Object.keys(a); const bKeys = Object.keys(b); - if (aKeys.length !== bKeys.length) return false; + + if (aKeys.length !== bKeys.length) { + return false; + } for (const key of aKeys) { - if (!Object.hasOwn(b, key)) return false; + if (!Object.hasOwn(b, key)) { + return false; + } } for (const key of aKeys) { - if (!equal(a[key as keyof typeof a], b[key as keyof typeof b], visited)) return false; + if (!equal(a[key as keyof typeof a], b[key as keyof typeof b], visited)) { + return false; + } } return true; } -function isObject(value: unknown): value is object { - return value != null && typeof value === 'object'; +/** Greedy suffices: `equal` is transitive. */ +function matchOneToOne( + left: Iterable, + right: Iterable, + visited: Visited, +): boolean { + const pool = [...right]; + + for (const a of left) { + const index = pool.findIndex((b) => equal(a, b, visited)); + + if (index < 0) { + return false; + } + pool.splice(index, 1); + } + + return true; } function isRegExp(value: unknown): value is RegExp { diff --git a/src/is-object.ts b/src/is-object.ts new file mode 100644 index 0000000..c425245 --- /dev/null +++ b/src/is-object.ts @@ -0,0 +1,3 @@ +export function isObject(value: unknown): value is Record { + return value !== null && typeof value === 'object'; +} diff --git a/src/react-props.tsx b/src/react-props.tsx index 8f4510d..f7af3a5 100644 --- a/src/react-props.tsx +++ b/src/react-props.tsx @@ -4,44 +4,40 @@ import { createComponent as _createComponent, type EventName, type Options } fro import { html } from 'lit'; import type React from 'react'; import { createPortal } from 'react-dom'; -import type { WithDataContext } from './backfills.js'; +import { type WithDataContext, withDataContext } from './backfills.js'; +import { isObject } from './is-object.js'; import { REQUEST_REMOVE, type RendererRequest, requestRenderer } from './render-props.js'; export type { EventName } from '@lit/react'; -type DistributiveOmit = T extends any +type DistributiveOmit = T extends any ? K extends keyof T ? Omit : T : T; type PropsWithoutRef = DistributiveOmit; -// A key value map matching React prop names to event names. +// React prop → event name. type EventNames = Record; -// A map of expected event listener types based on EventNames. type EventListeners = { [K in keyof R]?: R[K] extends EventName ? (e: R[K]['__eventType']) => void : (e: Event) => void; }; type ElementProps = Partial>; -// Acceptable props to the React component. type ComponentProps = Omit< React.HTMLAttributes, - // Prefer type of provided event handler props or those on element over - // built-in HTMLAttributes + // Element and event props win over HTMLAttributes. keyof E | keyof ElementProps > & EventListeners & ElementProps; -/** - * Mapped type to update the render props callback return type. - * - * A renderer entry is either a name (a template directly on the component) or a nested map of - * them (a template on a config object, as `igc-chat` declares its renderers), so it recurses. - */ +/** Prop name → renderer name, or a nested map for config props (e.g. `igc-chat`). */ +type Renderers = Record; + +/** Render props return `ReactNode`; nested maps recurse. */ type WithJsxRenderProps = { [K in keyof T]: K extends keyof R ? R[K] extends string @@ -64,28 +60,20 @@ export type ReactWebComponent< PropsWithoutRef, R>> & React.RefAttributes >; -/** A map of prop names to renderer names, nested to mirror the shape of the props themselves. */ -type Renderers = Record; - -/** The React module, which is injected rather than imported so Preact can be swapped in. */ type ReactModule = typeof React; type Props = Record; type RenderProp = (data: unknown) => React.ReactNode; +type Template = (ctx: unknown) => unknown; -/** A patched template handed to the element, and the renderer it stands in for. */ -type PatchedTemplate = { - patched: (ctx: unknown) => unknown; - rendererName: string; -}; +type Reparenting = 'move-back' | 'none'; +type NgElement = HTMLElement & { ngElementStrategy?: { parentElement?: WeakRef } }; -/** A slot the element has asked us to fill, and the portal currently filling it. */ -type PortalSlot = { - name: string; - data: unknown; - node: Element; - callback: RenderProp | undefined; - portal: React.ReactPortal; +type PortalSlot = RendererRequest & { + callback?: RenderProp; + /** Boxed: `reason` may be `undefined`. */ + failure?: { reason: unknown }; + portal?: React.ReactPortal; }; interface WrapperOptions @@ -94,149 +82,82 @@ interface WrapperOptions(); - - /** The *current* render prop callbacks, keyed by renderer name. */ - private readonly _callbacks = new Map(); - - /** Slots the element has asked us to fill, keyed by slot name. */ + private readonly _templates = new Map(); private readonly _slots = new Map(); - /** Nested prop containers, kept around so their identity is stable while their contents are. */ + /** Reused while shallow-equal: a new object updates the element. */ private readonly _containers = new Map(); - private readonly _renderers: Renderers; - private readonly _notify: () => void; - - constructor(renderers: Renderers, notify: () => void) { - this._renderers = renderers; - this._notify = notify; - } - - /** - * Turns the component props into the props handed to the element, swapping every render prop for - * a patched template of stable identity. - */ - public resolve(props: Props): Props { - const elementProps: Props = {}; + constructor( + private readonly _renderers: Renderers, + private readonly _notify: () => void, + ) {} - this._prune(props); - this._collect(props, this._renderers, elementProps); - this._refresh(); + public resolve(props: Props): { props: Props; portals: React.ReactPortal[] } { + const callbacks = new Map(); + const elementProps = this._collect(props, this._renderers, callbacks); - return elementProps; + return { props: elementProps, portals: this._portals(callbacks) }; } - /** The portals currently filling the element's slots. */ - public *portals(): Generator { - for (const { portal } of this._slots.values()) { - yield portal; - } - } - - /** - * Fills or clears a slot at the element's request. Bound once per instance, since the patched - * templates hold on to it for as long as the element does. - */ - private readonly _request = (req: RendererRequest): void => { + /** Arrow: templates call it unbound. */ + private readonly _request = (req: RendererRequest): void => { if (req.data === REQUEST_REMOVE) { this._slots.delete(req.slotName); } else { - const callback = this._callbacks.get(req.name); + const previous = this._slots.get(req.slotName); this._slots.set(req.slotName, { - name: req.name, - data: req.data, - node: req.node, - callback, - portal: createPortal(callback?.(req.data), req.node, req.slotName), + ...req, + data: withDataContext(req.data), + // Keep the old content until the new one renders. + portal: previous?.node === req.node ? previous.portal : undefined, }); } this._notify(); }; - /** - * Drops patched templates whose render prop is gone - either the prop was removed, or it is no - * longer a function. Both have to be pruned: leaving one behind hands the element a stale - * template, while recreating one on every render feeds it a new identity and spins the loop. - */ - private _prune(props: Props): void { - for (const [path, { rendererName }] of this._templates) { - if (typeof getAtPath(props, path) === 'function') { - continue; - } - - this._templates.delete(path); - this._callbacks.delete(rendererName); + private _collect( + props: Props, + renderers: Renderers, + callbacks: Map, + prefix = '', + ): Props { + const out: Props = {}; - // Drop the slots as well. The directive does emit a remove request when it disconnects, but - // it reaches for the callback through a `WeakRef` - once the patched template above is gone - // that request may never arrive, and the portal would linger for the component's lifetime. - for (const [slotName, slot] of this._slots) { - if (slot.name === rendererName) { - this._slots.delete(slotName); - } - } - } - } - - /** Copies the props over, swapping render props for templates and recursing into config objects. */ - private _collect(props: Props, renderers: Renderers, out: Props, prefix = ''): void { for (const prop in props) { const path = prefix ? `${prefix}.${prop}` : prop; const renderer = renderers[prop]; const value = props[prop]; - if (typeof renderer === 'string') { - out[prop] = this._template(path, renderer, value); - } else if (isRecord(renderer) && isRecord(value)) { - const nested: Props = {}; - - this._collect(value, renderer, nested, path); - out[prop] = this._container(path, nested); + if (typeof renderer === 'string' && typeof value === 'function') { + callbacks.set(renderer, value as RenderProp); + out[prop] = this._template(path, renderer); + } else if (isObject(renderer) && isObject(value)) { + out[prop] = this._container(path, this._collect(value, renderer, callbacks, path)); } else { + // Also non-function render props, e.g. `undefined`: the element uses its default. out[prop] = value; } } - } - - /** - * The patched template standing in for a render prop, created on first sight. - * - * Only a function is a template. Anything else - most often `undefined` from a conditional prop - - * has to reach the element untouched so it can fall back to its own default rendering. - */ - private _template(path: string, name: string, value: unknown): unknown { - if (typeof value !== 'function') { - return value; - } - // Refreshed on every render. The patched template is cached so the element sees a stable prop, - // but the callback it invokes must always be the current one, or the template renders against - // a stale closure. - this._callbacks.set(name, value as RenderProp); + return out; + } + /** Captures only the renderer name, so it never goes stale. */ + private _template(path: string, name: string): Template { let template = this._templates.get(path); if (!template) { - template = { patched: createPatched(this._request, name), rendererName: name }; + template = (ctx) => html`${requestRenderer(this._request, name, ctx)}`; this._templates.set(path, template); } - return template.patched; + return template; } - /** Reuses the previous container while its contents are unchanged, to keep its identity stable. */ private _container(path: string, next: Props): Props { const previous = this._containers.get(path); @@ -248,39 +169,75 @@ class TemplateBridge { return next; } + private _portals(callbacks: Map): React.ReactPortal[] { + const portals: React.ReactPortal[] = []; + + for (const slot of this._slots.values()) { + const callback = callbacks.get(slot.name); + + // Prop removed: element releases the slot. + if (!callback) { + continue; + } + + this._invoke(slot, callback); + + if (slot.failure) { + throw slot.failure.reason; + } + + if (slot.portal) { + portals.push(slot.portal); + } + } + + return portals; + } + /** - * A portal is built when the element requests its slot and reused as-is afterwards - handing - * React a fresh portal on every render would re-commit the template into the element, which - * re-renders the element, which requests the template again. - * - * It does have to be rebuilt when the render prop itself changes, though. Templates commonly - * close over state, and the element has no reason to re-request one just because the React tree - * above it re-rendered. + * New render prop re-invokes: it may close over state the element can't see. + * Same one keeps its portal; a fresh portal would re-commit and loop. */ - private _refresh(): void { - for (const [slotName, slot] of this._slots) { - const callback = this._callbacks.get(slot.name); + private _invoke(slot: PortalSlot, callback: RenderProp): void { + if (slot.callback === callback) { + return; + } - if (slot.callback !== callback) { - slot.callback = callback; - slot.portal = createPortal(callback?.(slot.data), slot.node, slotName); - } + const { node, slotName } = slot; + const content = callback(slot.data); + + slot.callback = callback; + slot.failure = undefined; + + if (!isThenable(content)) { + slot.portal = createPortal(content, node, slotName); + return; } + + // No Suspense: inline render props yield a new promise per render. + const settle = (patch: Partial) => { + if (this._slots.get(slotName) !== slot || slot.callback !== callback) { + return; + } + + Object.assign(slot, patch); + this._notify(); + }; + + content.then( + (resolved) => settle({ portal: createPortal(resolved, node, slotName) }), + (reason: unknown) => settle({ failure: { reason } }), + ); } } -/** Creates the render prop state of this component instance and keeps it for its lifetime. */ function useTemplateBridge(react: ReactModule, renderers: Renderers): TemplateBridge { - const [, forceUpdate] = react.useReducer(increment, 0); - const bridge = react.useRef(null); - - // `forceUpdate` is stable for the lifetime of the component, so capturing the first one is safe. - bridge.current ??= new TemplateBridge(renderers, forceUpdate); + const [, forceUpdate] = react.useReducer((n: number) => n + 1, 0); + const [bridge] = react.useState(() => new TemplateBridge(renderers, forceUpdate)); - return bridge.current; + return bridge; } -/** Forwards the ref while keeping a local handle on the element for the hooks that need one. */ function useForwardedRef( react: ReactModule, ref: React.ForwardedRef, @@ -303,54 +260,42 @@ function useForwardedRef( return [elementRef, setRef]; } -/** - * Handles element re-parenting for the Angular integration, where Angular Elements moves a - * projected element away from the parent React knows about. - */ -function useReparenting( +/** Moves the element back to its Angular Elements parent before React unmounts it. */ +function useReparenting( react: ReactModule, - enabled: boolean | undefined, - elementRef: React.RefObject, + mode: Reparenting, + elementRef: React.RefObject, ): void { const projectionParent = react.useRef | null>(null); - // https://react.dev/learn/reusing-logic-with-custom-hooks#keep-your-custom-hooks-focused-on-concrete-high-level-use-cases - // Runs once after first render. react.useLayoutEffect(() => { - if (!enabled) { + if (mode === 'none') { return; } - // already too late to save elementRef.current?.parentElement, rely on Elements - // secondary run (likely dev strict mode), move back to projection: + // Strict mode re-run: return to the projection parent. const prevParent = projectionParent.current?.deref(); if (prevParent && elementRef.current && prevParent !== elementRef.current.parentElement) { prevParent.appendChild(elementRef.current); + projectionParent.current = null; } - projectionParent.current = null; + // Runs before DOM removal. return () => { - // cleanup **before** component is removed from the DOM const element = elementRef.current; + const creationParent = element?.ngElementStrategy?.parentElement?.deref(); - if (!element) { + if (!element || !creationParent || creationParent === element.parentElement) { return; } - const creationParent = ( - element as I & { ngElementStrategy?: { parentElement?: WeakRef } } - ).ngElementStrategy?.parentElement?.deref(); - - if (creationParent && creationParent !== element.parentElement) { - // move back to original parent - if (element.parentElement) { - projectionParent.current = new WeakRef(element.parentElement); - } - creationParent.appendChild(element); + if (element.parentElement) { + projectionParent.current = new WeakRef(element.parentElement); } + creationParent.appendChild(element); }; - }, [enabled, elementRef]); + }, [mode, elementRef]); } export const createComponent = < @@ -366,7 +311,6 @@ export const createComponent = < renderProps, moveBackOnDelete, }: WrapperOptions): ReactWebComponent => { - // Register our components if ('register' in elementClass) { (elementClass as { register: () => void }).register(); } @@ -380,24 +324,22 @@ export const createComponent = < }); if (!renderProps && !moveBackOnDelete) { - // When R is empty (no renderProps), the component types are equivalent at runtime + // Runtime shape matches; only types differ. return component as unknown as ReactWebComponent; } const renderers: Renderers = renderProps ?? {}; + const reparenting: Reparenting = moveBackOnDelete ? 'move-back' : 'none'; - type PropsWithRenderProps = WithJsxRenderProps, R>; - - return React.forwardRef((props, ref) => { + return React.forwardRef, R>>((props, ref) => { const bridge = useTemplateBridge(React, renderers); - const [elementRef, setRef] = useForwardedRef(React, ref); - useReparenting(React, moveBackOnDelete, elementRef); + const [elementRef, setRef] = useForwardedRef(React, ref); + useReparenting(React, reparenting, elementRef); - const elementProps = bridge.resolve(props as Props); - const children = React.Children.toArray((props as { children?: React.ReactNode }).children); + const { props: elementProps, portals } = bridge.resolve(props as Props); + const { children } = props as { children?: React.ReactNode }; - children.push(...bridge.portals()); - elementProps.children = children; + elementProps.children = [...React.Children.toArray(children), ...portals]; return React.createElement(component, { ...elementProps, @@ -406,16 +348,8 @@ export const createComponent = < }); }; -function createPatched(callback: (req: RendererRequest) => void, propertyName: string) { - return (ctx: unknown) => html`${requestRenderer(callback, propertyName, ctx)}`; -} - -function increment(count: number): number { - return count + 1; -} - -function isRecord(value: unknown): value is Props { - return typeof value === 'object' && value !== null; +function isThenable(value: unknown): value is PromiseLike { + return isObject(value) && typeof value.then === 'function'; } function shallowEqual(a: Props, b: Props): boolean { @@ -427,17 +361,3 @@ function shallowEqual(a: Props, b: Props): boolean { return keys.every((key) => Object.is(a[key], b[key])); } - -/** Resolves a dot delimited path, returning `undefined` if any segment is missing. */ -function getAtPath(object: Props, path: string): unknown { - let current: unknown = object; - - for (const part of path.split('.')) { - if (!isRecord(current)) { - return undefined; - } - current = current[part]; - } - - return current; -} diff --git a/src/render-props.ts b/src/render-props.ts index 725bf6a..81d5d9a 100644 --- a/src/render-props.ts +++ b/src/render-props.ts @@ -5,90 +5,63 @@ import { type DirectiveParameters, directive, } from 'lit/async-directive.js'; -import { withDataContext } from './backfills.js'; import { equal } from './equal.js'; +import { isObject } from './is-object.js'; import { getUUID } from './random-uuid.js'; export const REQUEST_REMOVE = Symbol('renderer-remove'); const NOT_SET = Symbol('not-set'); -type NgState = T & { implicit: unknown }; -type RendererState = { - previous: T; - current: T; -}; - -function createRequestData( - name: string, - data: T | typeof REQUEST_REMOVE, - node: Element, - key?: string, -): RendererRequest { - return { - name, - data: data === REQUEST_REMOVE ? data : withDataContext(data), - slotName: key !== undefined ? `${name}${key}` : name, - node, - }; -} - -export type RendererRequest = { - data: T | typeof REQUEST_REMOVE; +export type RendererRequest = { + data: unknown; name: string; slotName: string; node: Element; }; -type RendererCallback = (req: RendererRequest) => unknown; +type RendererCallback = (req: RendererRequest) => unknown; -class RequestRenderer extends AsyncDirective { +class RequestRenderer extends AsyncDirective { private readonly _key = getUUID(); private _part: WeakRef | null = null; - private _callback: WeakRef> | null = null; - - private _state = { previous: NOT_SET, current: undefined } as RendererState; + private _callback: WeakRef | null = null; private _name!: string; + private _data: unknown; + private _previous: unknown = NOT_SET; - private get _renderNode(): Element | undefined { - return this._part?.deref()?.parentNode as Element | undefined; - } - - private _shouldUpdateNG(_data: NgState): boolean { - /* Can't compare implicit, in main use case it'd be the cell value which might repeat, - be undefined or otherwise unrelated to the template content. Disabled for now. Reevaluate: */ + private _shouldUpdate(): boolean { + // Angular context: `implicit` (e.g. cell value) may repeat; always update. + if (isObject(this._data) && 'implicit' in this._data) { + return true; + } - // if (equal(data.$implicit, this._state.previous)) { - // return false; - // } + if (equal(this._previous, this._data)) { + return false; + } - // this._state.previous = data.$implicit as T; + this._previous = this._data; return true; } - private _shouldUpdate(): boolean { - const data = this._state.current; - - if (data !== null && typeof data === 'object' && Reflect.has(data as NgState, 'implicit')) { - return this._shouldUpdateNG(data as NgState); - } + private _request(callback: RendererCallback, data: unknown): void { + const node = this._part?.deref()?.parentNode as Element | undefined; - if (equal(this._state.previous, data)) { - return false; + if (!node) { + return; } - this._state.previous = data; - return true; + callback({ name: this._name, data, slotName: `${this._name}${this._key}`, node }); } - /** Dispatches a request for the current state, if there is somewhere to render it. */ - private _request(callback: RendererCallback, data: T | typeof REQUEST_REMOVE): void { - const node = this._renderNode; - if (!node) return; + private _sync(): void { + const callback = this._callback?.deref(); - callback(createRequestData(this._name, data, node, this._key)); + if (callback && this._shouldUpdate()) { + this._request(callback, this._data); + } } - public override render(_callback: RendererCallback, _name: string, _data: T): symbol { + public override render(_callback: RendererCallback, _name: string, _data: unknown): symbol { return noChange; } @@ -96,32 +69,34 @@ class RequestRenderer extends AsyncDirective { part: ChildPart, [callback, name, data]: DirectiveParameters, ): symbol { - this._callback = new WeakRef(callback); + if (this._callback?.deref() !== callback) { + this._callback = new WeakRef(callback); + } + + this._part ??= new WeakRef(part); this._name = name; - this._state.current = data; - this._part = new WeakRef(part); + this._data = data; - if (this.isConnected && callback && this._shouldUpdate()) { - this._request(callback, this._state.current); + if (this.isConnected) { + this._sync(); } return noChange; } protected override reconnected(): void { - const callback = this._callback?.deref(); - if (callback && this._shouldUpdate()) { - this._request(callback, this._state.current); - } + this._sync(); } protected override disconnected(): void { const callback = this._callback?.deref(); + if (callback) { this._request(callback, REQUEST_REMOVE); } - // drop prev, so a reconnect would behave like initial - this._state.previous = NOT_SET as T; + + // Reconnect acts like first render. + this._previous = NOT_SET; } } diff --git a/tests/backfills.spec.ts b/tests/backfills.spec.ts new file mode 100644 index 0000000..713ade1 --- /dev/null +++ b/tests/backfills.spec.ts @@ -0,0 +1,9 @@ +import { describe, expect, it } from 'vitest'; +import { withDataContext } from '../src/backfills'; + +describe('withDataContext', () => { + it('passes non-object contexts through', () => { + expect(withDataContext(undefined)).toBeUndefined(); + expect(withDataContext(1)).toBe(1); + }); +}); diff --git a/tests/equal.spec.ts b/tests/equal.spec.ts index 8d4f1fa..9696937 100644 --- a/tests/equal.spec.ts +++ b/tests/equal.spec.ts @@ -337,34 +337,55 @@ describe('equal', () => { const set2 = new Set([]); expect(equal(set1, set2)).toBe(false); }); + it('should not let failed Set candidate pairs poison later comparisons', () => { const a = new Set([{ x: 1 }, { x: 2 }]); const b = new Set([{ x: 2 }, { x: 1 }]); expect(equal(a, b)).toBe(true); - // `{ k: 'b' }` has no counterpart, so these Sets differ. Getting there walks candidate pairs - // that fail, and those failures must not be remembered as matches. + // `{ k: 'b' }` has no counterpart. const left = new Set([{ k: 'z' }, { k: 'b' }, { k: 'x' }]); const right = new Set([{ k: 'x' }, { k: 'y' }, { k: 'z' }]); expect(equal(left, right)).toBe(false); }); it('should not let failed Map candidate pairs poison later comparisons', () => { - const a = new Map([ - ['a', { v: 1 }], - ['b', { v: 2 }], + // `{ k: 'b' }` has no counterpart. + const left = new Map([ + [{ k: 'z' }, 1], + [{ k: 'b' }, 1], + [{ k: 'x' }, 1], ]); - const b = new Map([ - ['b', { v: 2 }], - ['a', { v: 1 }], + const right = new Map([ + [{ k: 'x' }, 1], + [{ k: 'y' }, 1], + [{ k: 'z' }, 1], ]); - expect(equal(a, b)).toBe(true); + expect(equal(left, right)).toBe(false); + }); + + it('should match Set elements one to one', () => { + expect(equal(new Set([{ x: 1 }, { x: 1 }]), new Set([{ x: 1 }, { x: 2 }]))).toBe(false); + expect(equal(new Set([{ x: 1 }, { x: 1 }]), new Set([{ x: 1 }, { x: 1 }]))).toBe(true); + }); - const mismatched = new Map([ - ['a', { v: 1 }], - ['b', { v: 3 }], + it('should match Map entries one to one', () => { + const left = new Map([ + [{ k: 1 }, 'v'], + [{ k: 1 }, 'v'], ]); - expect(equal(a, mismatched)).toBe(false); + const right = new Map([ + [{ k: 1 }, 'v'], + [{ k: 2 }, 'v'], + ]); + expect(equal(left, right)).toBe(false); + }); + + it('should compare null-prototype objects', () => { + const create = (value: number) => Object.assign(Object.create(null), { a: value }); + + expect(equal(create(1), create(1))).toBe(true); + expect(equal(create(1), create(2))).toBe(false); }); it('should still terminate on circular references', () => { diff --git a/tests/grid-lite/Templates.spec.tsx b/tests/grid-lite/Templates.spec.tsx index 09eb960..afa1131 100644 --- a/tests/grid-lite/Templates.spec.tsx +++ b/tests/grid-lite/Templates.spec.tsx @@ -1,7 +1,7 @@ -import { expect, test } from 'vitest'; +import { expect, test, vi } from 'vitest'; import { page } from 'vitest/browser'; import { render } from 'vitest-browser-react'; -import { OptionalTemplate, StatefulTemplate } from './Templates'; +import { AsyncTemplate, FailingTemplate, OptionalTemplate, StatefulTemplate } from './Templates'; const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); @@ -12,29 +12,15 @@ test('render props observe updated React state', async () => { await page.getByRole('button', { name: 'Increment' }).click(); - // the template closes over `count` - it must re-render with the new value await expect.element(page.getByText('V:1/C:1', { exact: true })).toBeVisible(); await expect.element(page.getByText('V:2/C:1', { exact: true })).toBeVisible(); }); -test('a render prop passed as undefined leaves its siblings intact', async () => { +test('a render prop passed as undefined falls back to the default rendering', async () => { render(); await expect.element(page.getByText('V:1', { exact: true })).toBeVisible(); - await expect.element(page.getByText('V:2', { exact: true })).toBeVisible(); -}); - -test('a render prop passed as undefined does not spin the render loop', async () => { - render(); - - const cell = page.getByText('V:1', { exact: true }); - await expect.element(cell).toBeVisible(); - - // portals are torn down and recreated on every cycle, so a settled component - // keeps the same DOM node - const node = cell.element(); - await delay(300); - expect(cell.element()).toBe(node); + await expect.element(page.getByText('id', { exact: true })).toBeVisible(); }); test('a render prop added on a later render does not evict the existing ones', async () => { @@ -48,3 +34,39 @@ test('a render prop added on a later render does not evict the existing ones', a await expect.element(page.getByText('V:1', { exact: true })).toBeVisible(); await expect.element(page.getByText('V:2', { exact: true })).toBeVisible(); }); + +test('an async render prop keeps its content until the new one settles', async () => { + let gate = Promise.withResolvers(); + const wait = vi.fn(() => gate.promise); + const open = () => { + gate.resolve(); + gate = Promise.withResolvers(); + }; + + render(); + + await expect.poll(() => wait.mock.calls.length).toBeGreaterThan(0); + open(); + await expect.element(page.getByText('A:1/C:0', { exact: true })).toBeVisible(); + + const initial = wait.mock.calls.length; + await page.getByRole('button', { name: 'Increment' }).click(); + + await expect.element(page.getByText('Count:1', { exact: true })).toBeVisible(); + await expect.element(page.getByText('A:1/C:0', { exact: true })).toBeVisible(); + + await expect.poll(() => wait.mock.calls.length).toBeGreaterThan(initial); + open(); + await expect.element(page.getByText('A:1/C:1', { exact: true })).toBeVisible(); + + // No render loop. + const settled = wait.mock.calls.length; + await delay(300); + expect(wait).toHaveBeenCalledTimes(settled); +}); + +test('an async render prop that rejects reaches the error boundary', async () => { + render(); + + await expect.element(page.getByText('Caught:boom', { exact: true })).toBeVisible(); +}); diff --git a/tests/grid-lite/Templates.tsx b/tests/grid-lite/Templates.tsx index d16d786..daca8e1 100644 --- a/tests/grid-lite/Templates.tsx +++ b/tests/grid-lite/Templates.tsx @@ -1,4 +1,4 @@ -import { useMemo, useState } from 'react'; +import { Component, type ComponentProps, type ReactNode, useState } from 'react'; import { type IgrCellContext, IgrGridLite, @@ -17,10 +17,19 @@ const data: Person[] = [ { id: 2, name: 'Bob' }, ]; -/** A cell template closing over React state. */ +type Templates = Pick, 'cellTemplate' | 'headerTemplate'>; + +function PeopleGrid(templates: Templates) { + return ( + + + + + ); +} + export function StatefulTemplate() { const [count, setCount] = useState(0); - const records = useMemo(() => data, []); const cellTemplate = (ctx: IgrCellContext) => ( @@ -30,10 +39,7 @@ export function StatefulTemplate() { return ( <> - - - - + @@ -41,28 +47,71 @@ export function StatefulTemplate() { ); } -/** A render prop that is conditionally `undefined`, next to one that is always set. */ export function OptionalTemplate() { const [withHeader, setWithHeader] = useState(false); - const records = useMemo(() => data, []); const cellTemplate = (ctx: IgrCellContext) => V:{ctx.value}; const headerTemplate = (ctx: IgrHeaderContext) => H:{ctx.column.field}; return ( <> - - - - + ); } + +export function AsyncTemplate({ wait }: { wait: () => Promise }) { + const [count, setCount] = useState(0); + + const cellTemplate = async (ctx: IgrCellContext) => { + await wait(); + + return ( + + A:{ctx.value}/C:{count} + + ); + }; + + return ( + <> + + Count:{count} + + + ); +} + +type BoundaryState = { error?: Error }; + +class Boundary extends Component<{ children: ReactNode }, BoundaryState> { + state: BoundaryState = {}; + + static getDerivedStateFromError(error: Error) { + return { error }; + } + + render() { + return this.state.error ?

Caught:{this.state.error.message}

: this.props.children; + } +} + +export function FailingTemplate() { + const cellTemplate = async () => { + throw new Error('boom'); + }; + + return ( + + + + ); +} diff --git a/tests/web-components/Chat.spec.tsx b/tests/web-components/Chat.spec.tsx index e4b9160..b34ab82 100644 --- a/tests/web-components/Chat.spec.tsx +++ b/tests/web-components/Chat.spec.tsx @@ -2,7 +2,7 @@ import { afterAll, expect, test, vi } from 'vitest'; import { locators, page, userEvent } from 'vitest/browser'; import { render } from 'vitest-browser-react'; -import Chat from './Chat'; +import Chat, { ToggledHeader } from './Chat'; afterAll(() => vi.restoreAllMocks()); @@ -67,6 +67,16 @@ test('Markdown support with default renderer', async () => { expect(message.element().tagName).toMatch(/a/i); }); +test('a renderer restored before the chat re-renders keeps its content', async () => { + render(); + + await expect.element(page.getByText('HDR', { exact: true })).toBeVisible(); + + await page.getByRole('button', { name: 'Toggle' }).click(); + + await expect.element(page.getByText('HDR', { exact: true })).toBeVisible(); +}); + //#region Locator extension locators.extend({ getByPart(part: string) { diff --git a/tests/web-components/Chat.tsx b/tests/web-components/Chat.tsx index 5890a59..21fd358 100644 --- a/tests/web-components/Chat.tsx +++ b/tests/web-components/Chat.tsx @@ -1,3 +1,5 @@ +import { useState } from 'react'; +import { flushSync } from 'react-dom'; import { type ChatMessageRenderContext, IgrChat } from '../../src/components'; import '../../node_modules/igniteui-webcomponents/themes/light/bootstrap.css'; import { createChatMarkdownRenderer } from '../../src/extras/index'; @@ -44,3 +46,25 @@ export default function BasicForm() { > ); } + +export function ToggledHeader() { + const [enabled, setEnabled] = useState(true); + + const messages = [{ id: '1', text: 'Hello', sender: 'support', timestamp: '0' }]; + const messageHeader = () => HDR; + + // Both commits land before the element's queued update. + const toggle = () => { + flushSync(() => setEnabled(false)); + flushSync(() => setEnabled(true)); + }; + + return ( + <> + + + + ); +}