From 5c94e9a27ed74e23ed3298435dd652050642969f Mon Sep 17 00:00:00 2001 From: David Matejka Date: Thu, 2 Jul 2026 18:13:24 +0200 Subject: [PATCH] feat(bindx-react): createComponent().use() + crash-proof static selection analysis MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #57 — using a scalar prop value (a translator call, a labels object) in a createComponent render body crashed implicit-selection collection with "t is not a function", and one crashing component silently dropped its siblings' selections from the fetch plan. Implicit collection is an abstract interpretation of the render body; this makes it total and confines it to the static-analysis phase: - Scalar props get a tolerant stand-in during collection (callable, property-safe, primitive-coercible, iterable) instead of undefined, so render bodies using them survive and entity fields keep being captured. - ComponentImpl no longer triggers collection — it runs only from the static surface (getSelection, $propName fragment getters, interfaces proxy). Render bodies are pure runtime code; components whose selection is never consumed statically are never collected. - A throw during collection no longer costs the whole component (scopes capture eagerly; the partial selection is finalized on the throw path) and is reported loudly with component attribution instead of being swallowed. analyzeJsx catches per node, so a broken component no longer drops sibling selections. - New .use(fn) builder step: runtime-only values with hooks allowed (useT(), useContext(), ...). Runs inside the React render and merges into render props; chainable, later fns see earlier values. Static analysis skips it entirely — its outputs are covered by the scalar stand-ins. This removes the need for thin wrapper components that only thread hook-derived values in as props (and which also broke selection discovery, because the JSX walk cannot see through plain components). Repro test from the issue branch + .use() test suite included. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_016eijo87HdS4vA6D2ZcS6KL --- packages/bindx-react/src/jsx/analyzer.ts | 39 +++- .../bindx-react/src/jsx/componentBuilder.ts | 19 ++ .../src/jsx/componentBuilder.types.ts | 69 ++++++- .../bindx-react/src/jsx/componentFactory.ts | 112 ++++++++--- ...eateComponentScalarPropCollection.test.tsx | 60 ++++++ tests/react/jsx/createComponentUse.test.tsx | 185 ++++++++++++++++++ 6 files changed, 446 insertions(+), 38 deletions(-) create mode 100644 tests/react/jsx/createComponentScalarPropCollection.test.tsx create mode 100644 tests/react/jsx/createComponentUse.test.tsx diff --git a/packages/bindx-react/src/jsx/analyzer.ts b/packages/bindx-react/src/jsx/analyzer.ts index ab3201e4..f2d087f8 100644 --- a/packages/bindx-react/src/jsx/analyzer.ts +++ b/packages/bindx-react/src/jsx/analyzer.ts @@ -60,8 +60,15 @@ export function analyzeJsx(node: ReactNode, selection: SelectionMetaCollector): return nestedSelection } - // Get selection info from the component - const fieldSelection = provider.getSelection(element.props, collectNested) + // Get selection info from the component. A throw here must not silently + // drop the sibling components' selections — report it and continue. + let fieldSelection: ReturnType + try { + fieldSelection = provider.getSelection(element.props, collectNested) + } catch (error) { + reportAnalysisError(component, error) + return + } // Add to selection if (fieldSelection) { @@ -85,11 +92,15 @@ export function analyzeJsx(node: ReactNode, selection: SelectionMetaCollector): 'staticRender' in component && typeof (component as { staticRender: unknown }).staticRender === 'function' ) { - const staticJsx = (component as { staticRender: (props: Record) => ReactNode }).staticRender( - element.props as Record, - ) - if (staticJsx) { - analyzeJsx(staticJsx, selection) + try { + const staticJsx = (component as { staticRender: (props: Record) => ReactNode }).staticRender( + element.props as Record, + ) + if (staticJsx) { + analyzeJsx(staticJsx, selection) + } + } catch (error) { + reportAnalysisError(component, error) } return } @@ -102,6 +113,20 @@ export function analyzeJsx(node: ReactNode, selection: SelectionMetaCollector): } } +/** + * Reports a selection-analysis failure of a single component with attribution, + * so one broken component doesn't silently cost its siblings their selection. + */ +function reportAnalysisError(component: unknown, error: unknown): void { + const c = component as { displayName?: string; name?: string; type?: { displayName?: string; name?: string } } + const name = c.displayName ?? c.type?.displayName ?? (c.name || c.type?.name) ?? 'anonymous' + console.error( + `[bindx] Selection analysis of <${name}> failed — its fields were NOT added to the fetch plan. ` + + 'Fix the error below; fields it uses may be missing from queries until then.', + error, + ) +} + /** * Collects all field selections from a JSX tree. * Entry point for the collection phase. diff --git a/packages/bindx-react/src/jsx/componentBuilder.ts b/packages/bindx-react/src/jsx/componentBuilder.ts index cf761b19..045ab966 100644 --- a/packages/bindx-react/src/jsx/componentBuilder.ts +++ b/packages/bindx-react/src/jsx/componentBuilder.ts @@ -49,6 +49,7 @@ export class ComponentBuilderImpl< private readonly hasInterfacesMode: boolean = false, private readonly conditionFn: ((props: Record) => Condition) | null = null, private readonly slotNames: readonly string[] = ['children'], + private readonly useFns: readonly ((props: Record) => object)[] = [], ) {} entity( @@ -67,6 +68,7 @@ export class ComponentBuilderImpl< this.hasInterfacesMode, this.conditionFn, this.slotNames, + this.useFns, ) } @@ -96,6 +98,7 @@ export class ComponentBuilderImpl< true, // Enable interfaces mode for discovery of implicit interface props this.conditionFn, this.slotNames, + this.useFns, ) } @@ -108,6 +111,19 @@ export class ComponentBuilderImpl< this.hasInterfacesMode, this.conditionFn, this.slotNames, + this.useFns, + ) + } + + use(useFn: (props: Record) => object): ComponentBuilderImpl { + return new ComponentBuilderImpl( + this.schemaRegistry, + this.entityConfigs, + this.roles, + this.hasInterfacesMode, + this.conditionFn, + this.slotNames, + [...this.useFns, useFn], ) } @@ -119,6 +135,7 @@ export class ComponentBuilderImpl< this.hasInterfacesMode, conditionFn, this.slotNames, + this.useFns, ) } @@ -130,6 +147,7 @@ export class ComponentBuilderImpl< this.hasInterfacesMode, this.conditionFn, names, + this.useFns, ) } @@ -142,6 +160,7 @@ export class ComponentBuilderImpl< this.schemaRegistry, this.conditionFn, this.slotNames, + this.useFns, ) } } diff --git a/packages/bindx-react/src/jsx/componentBuilder.types.ts b/packages/bindx-react/src/jsx/componentBuilder.types.ts index e58313d0..3215ee48 100644 --- a/packages/bindx-react/src/jsx/componentBuilder.types.ts +++ b/packages/bindx-react/src/jsx/componentBuilder.types.ts @@ -107,10 +107,13 @@ export interface ComponentBuilderState< TEntityProps extends Record = {}, TScalarProps extends object = object, TRoles extends readonly string[] = readonly string[], + // eslint-disable-next-line @typescript-eslint/ban-types + TUseProps extends object = {}, > { readonly __entityProps: TEntityProps readonly __scalarProps: TScalarProps readonly __roles: TRoles + readonly __useProps: TUseProps } // ============================================================================ @@ -130,7 +133,8 @@ export type AddImplicitEntity< readonly [K in TPropName]: ImplicitEntityConfig }, TState['__scalarProps'], - TState['__roles'] + TState['__roles'], + TState['__useProps'] > /** @@ -147,7 +151,8 @@ export type AddExplicitEntity< readonly [K in TPropName]: ExplicitEntityConfig }, TState['__scalarProps'], - TState['__roles'] + TState['__roles'], + TState['__useProps'] > /** @@ -162,7 +167,8 @@ export type AddImplicitInterfaceEntity< readonly [K in TPropName]: ImplicitInterfaceEntityConfig }, TState['__scalarProps'], - TState['__roles'] + TState['__roles'], + TState['__useProps'] > /** @@ -177,7 +183,8 @@ export type AddExplicitInterfaceEntity< readonly [K in TPropName]: ExplicitInterfaceEntityConfig }, TState['__scalarProps'], - TState['__roles'] + TState['__roles'], + TState['__useProps'] > /** @@ -192,7 +199,8 @@ export type AddInterfaces< readonly [K in keyof TInterfaces]: InterfaceEntityPropConfig }, TState['__scalarProps'], - TState['__roles'] + TState['__roles'], + TState['__useProps'] > /** @@ -214,7 +222,21 @@ export type SetScalarProps< > = ComponentBuilderState< TState['__entityProps'], TNewScalarProps, - TState['__roles'] + TState['__roles'], + TState['__useProps'] +> + +/** + * Merge runtime-only values from .use() into builder state. + */ +export type AddUseProps< + TState extends ComponentBuilderState, + TUse extends object, +> = ComponentBuilderState< + TState['__entityProps'], + TState['__scalarProps'], + TState['__roles'], + TState['__useProps'] & TUse > // ============================================================================ @@ -273,9 +295,12 @@ export type BuildProps = /** * Build complete render props type from builder state. + * Includes .use() values — they exist only inside the render function, + * never on the public props (BuildProps). */ export type BuildRenderProps = TState['__scalarProps'] & + TState['__useProps'] & BuildRenderEntityProps /** @@ -425,6 +450,38 @@ export interface ComponentBuilder< */ props(): ComponentBuilder> + /** + * Provide runtime-only values to the render function — hooks are allowed here. + * + * The function runs during every React render (inside the component), so + * `useT()`, `useContext()`, `useMemo()` etc. all work. It is NEVER executed + * during static selection analysis — its outputs are replaced by inert + * stand-ins there, so the render body can use them freely without crashing + * the collection pass. + * + * This removes the need for thin wrapper components that only thread + * hook-derived values in as props (which also break selection discovery, + * because the JSX walk cannot see through plain components). + * + * Can be chained; later use() functions see the values of earlier ones. + * + * @example + * ```typescript + * createComponent() + * .entity('article', schema.Article) + * .use(() => ({ t: useTranslator() })) + * .render(({ article, t }) => ( + *
+ *

{t('article.heading')}

+ * + *
+ * )) + * ``` + */ + use( + useFn: (props: BuildRenderProps) => TUse, + ): ComponentBuilder> + /** * Add a condition that must be true for the component to render. * If the condition is false, the component renders null. diff --git a/packages/bindx-react/src/jsx/componentFactory.ts b/packages/bindx-react/src/jsx/componentFactory.ts index f7c7f8d2..af488dd2 100644 --- a/packages/bindx-react/src/jsx/componentFactory.ts +++ b/packages/bindx-react/src/jsx/componentFactory.ts @@ -101,8 +101,10 @@ export function buildComponent( schemaRegistry: SchemaRegistry> | null, conditionFn: ((props: TProps) => Condition) | null, slotNames: readonly string[], + useFns: readonly ((props: TProps) => object)[], ): unknown { const selectionsMap = new Map() + const componentDisplayName = `BindxComponent(${[...entityConfigs.keys()].join(', ')})` // Generate unique brand for this component const componentBrand = new ComponentBrand(`component_${Math.random().toString(36).slice(2)}`) @@ -128,28 +130,48 @@ export function buildComponent( // 2. Implicit entities - collect lazily to avoid TDZ errors const implicitConfigs = [...entityConfigs.entries()].filter(([_, c]) => !c.selector) - let implicitCollected = false + // Tri-state: 'collecting' terminates self-recursive components + let collectionState: 'idle' | 'collecting' | 'done' = 'idle' + // Runs only from the static-analysis surface (getSelection, $propName fragment + // getters) — never from render. Render bodies stay pure runtime code. function ensureImplicitCollected(): void { // In interfaces mode, we also need to collect even if no explicit implicit configs exist // (because interface props may be discovered dynamically) // Also collect if there's a conditionFn (it may access entity fields) - if (implicitCollected || (implicitConfigs.length === 0 && !hasInterfacesMode && !conditionFn)) { + if (collectionState !== 'idle' || (implicitConfigs.length === 0 && !hasInterfacesMode && !conditionFn)) { return } - implicitCollected = true - collectImplicitSelections(implicitConfigs, renderFn, selectionsMap, componentBrand, roles, hasInterfacesMode, schemaRegistry, conditionFn) + collectionState = 'collecting' + try { + collectImplicitSelections(implicitConfigs, renderFn, selectionsMap, componentBrand, roles, hasInterfacesMode, schemaRegistry, conditionFn) + } catch (error) { + // Analysis is deterministic, so retrying is pointless — degrade loudly + // to the fields captured before the throw (scopes record eagerly). + console.error( + `[bindx] Implicit selection analysis of <${componentDisplayName}> failed — ` + + 'only fields accessed before the error were collected; fields used after it may be missing from queries.', + error, + ) + } finally { + collectionState = 'done' + } } // 3. Create React component function ComponentImpl(props: TProps): ReactNode { - ensureImplicitCollected() - // Convert explicit entity refs to accessors (stable hook count — explicitEntityPropNames is fixed) - const renderProps = explicitEntityPropNames.length > 0 + let renderProps = explicitEntityPropNames.length > 0 ? useRenderProps(props, explicitEntityPropNames) : props + // Runtime-only values from .use() — hooks are allowed here; static analysis + // never executes these (their outputs are mocked in the collection pass). + for (const useFn of useFns) { + // eslint-disable-next-line react-hooks/rules-of-hooks -- stable iteration count (useFns is fixed at build time) + renderProps = { ...renderProps, ...useFn(renderProps) } + } + // Evaluate condition at runtime if (conditionFn) { const condition = conditionFn(renderProps) @@ -161,6 +183,8 @@ export function buildComponent( } const MemoizedComponent = memo(ComponentImpl) + // Names for selection-analysis error attribution; users can override + MemoizedComponent.displayName = componentDisplayName // 4. Attach metadata const comp = MemoizedComponent as unknown as Record @@ -262,6 +286,34 @@ function createExplicitPropMock(): unknown { }) } +/** + * Creates a tolerant stand-in for scalar (non-entity) props during collection. + * Render bodies may call it (`t('key')`), read nested properties + * (`labels.heading`) or coerce it to a primitive — all are no-ops so the + * collection pass keeps capturing entity field accesses (see issue #57). + */ +function createScalarPropMock(): unknown { + const mock: unknown = new Proxy(function () {}, { + get(_target, prop): unknown { + if (prop === Symbol.toPrimitive || prop === 'toString' || prop === 'valueOf') { + return (): string => '' + } + if (prop === Symbol.iterator) { + return function* (): Generator {} + } + // undefined keeps JSON.stringify from recursing via a callable toJSON + if (prop === 'toJSON') { + return undefined + } + return mock + }, + apply(): unknown { + return mock + }, + }) + return mock +} + /** * Collects selections from JSX for implicit entity props. * @@ -327,18 +379,38 @@ function collectImplicitSelections( return createCollectorProxy(scope, null, schemaRegistry) } - // Scalar prop - return undefined - return undefined + // Scalar prop — tolerant stand-in so render bodies using it survive + return createScalarPropMock() }, }) - // Execute conditionFn to capture field accesses from the condition - if (conditionFn) { - conditionFn(propsProxy) + // Create fragments for captured entities + const finalizeScopes = (): void => { + for (const [propName, scope] of propScopes) { + if (scope.hasFields()) { + const selection = scope.toSelectionMeta() + + selectionsMap.set(propName, { + selection, + fragment: createFragment(selection, componentBrand, roles), + }) + } + } } - // Execute render to capture field accesses - const jsx = renderFn(propsProxy) + let jsx: ReactNode + try { + // Execute conditionFn + render to capture field accesses + if (conditionFn) { + conditionFn(propsProxy) + } + jsx = renderFn(propsProxy) + } catch (error) { + // Scopes capture accesses eagerly, so a throw mid-render still leaves a + // usable partial selection — keep it, then let the caller report the error. + finalizeScopes() + throw error + } // Analyze JSX tree for component-level selections (handles nested createComponent) try { @@ -349,17 +421,7 @@ function collectImplicitSelections( // Field accesses are still captured in the scope tree via proxy. } - // Create fragments for captured entities - for (const [propName, scope] of propScopes) { - if (scope.hasFields()) { - const selection = scope.toSelectionMeta() - - selectionsMap.set(propName, { - selection, - fragment: createFragment(selection, componentBrand, roles), - }) - } - } + finalizeScopes() } // ============================================================================ diff --git a/tests/react/jsx/createComponentScalarPropCollection.test.tsx b/tests/react/jsx/createComponentScalarPropCollection.test.tsx new file mode 100644 index 00000000..7aae1e40 --- /dev/null +++ b/tests/react/jsx/createComponentScalarPropCollection.test.tsx @@ -0,0 +1,60 @@ +// Regression test for https://github.com/contember/bindx/issues/57 +import '../../setup' +import { describe, test, expect } from 'bun:test' +import React from 'react' +import { createComponent, Field, COMPONENT_SELECTIONS, type SelectionMeta } from '@contember/bindx-react' +import { schema } from '../../shared' + +// Triggers lazy implicit-selection collection by reading the `$` +// fragment getter (same mechanism the parent collector uses). +function getComponentSelection(component: unknown, propName: string): SelectionMeta | undefined { + const fragment = (component as Record)[`$${propName}`] + if (!fragment) return undefined + const selections = (component as Record>)[COMPONENT_SELECTIONS] + return selections?.get(propName)?.selection +} + +function getFieldNames(selection: SelectionMeta): string[] { + return [...selection.fields.keys()] +} + +describe('createComponent — scalar prop used in render body during collection', () => { + // During implicit-selection collection bindx runs the render body with a + // proxy whose scalar (`.props<>()`) values are `undefined`, and the + // `renderFn(propsProxy)` pass is unguarded. So a render body that uses a + // scalar prop value — e.g. calling a translator `t(key)` — crashes the + // whole collection with "t is not a function", even though this is correct + // usage (no hook, no `.value` read on a ref). + test('should not crash collection when render body calls a scalar function prop', () => { + const Comp = createComponent() + .entity('entity', schema.Article) + .props<{ t: (key: string) => string }>() + .render(({ entity, t }) => ( +
+

{t('some.heading')}

+ +
+ )) + + expect(() => getComponentSelection(Comp, 'entity')).not.toThrow() + + // Collection must still discover the entity fields. + const sel = getComponentSelection(Comp, 'entity') + expect(sel).toBeDefined() + expect(getFieldNames(sel!)).toContain('title') + }) + + test('should not crash collection when render body reads a scalar object prop', () => { + const Comp = createComponent() + .entity('entity', schema.Article) + .props<{ labels: { heading: string } }>() + .render(({ entity, labels }) => ( +
+

{labels.heading}

+ +
+ )) + + expect(() => getComponentSelection(Comp, 'entity')).not.toThrow() + }) +}) diff --git a/tests/react/jsx/createComponentUse.test.tsx b/tests/react/jsx/createComponentUse.test.tsx new file mode 100644 index 00000000..ccbd6900 --- /dev/null +++ b/tests/react/jsx/createComponentUse.test.tsx @@ -0,0 +1,185 @@ +// Tests for createComponent().use() — runtime-only values (hooks allowed) that +// static selection analysis never executes. See issue #57 for the motivation. +import '../../setup' +import { describe, test, expect, afterEach } from 'bun:test' +import { cleanup, waitFor } from '@testing-library/react' +import React, { createContext, useContext } from 'react' +import { createComponent, Field, Entity, COMPONENT_SELECTIONS, type SelectionMeta } from '@contember/bindx-react' +import { schema, renderWithBindx, getByTestId } from '../../shared' + +afterEach(() => { + cleanup() +}) + +// Triggers static collection via the `$` fragment getter (same +// mechanism the parent Entity walk uses through getSelection). +function getComponentSelection(component: unknown, propName: string): SelectionMeta | undefined { + const fragment = (component as Record)[`$${propName}`] + if (!fragment) return undefined + const selections = (component as Record>)[COMPONENT_SELECTIONS] + return selections?.get(propName)?.selection +} + +describe('createComponent().use()', () => { + test('static analysis skips use(); runtime render receives its values', async () => { + let useCalls = 0 + const Comp = createComponent() + .entity('article', schema.Article) + .use(() => { + useCalls++ + return { t: (key: string): string => `translated:${key}` } + }) + .render(({ article, t }) => ( +
+

{t('heading')}

+ +
+ )) + + // Fragment access = static analysis; use() must not run, fields must be found + const selection = getComponentSelection(Comp, 'article') + expect(useCalls).toBe(0) + expect(selection).toBeDefined() + expect([...selection!.fields.keys()]).toContain('title') + + // End-to-end: field fetched through the Entity walk, use() value rendered + const { container } = renderWithBindx( + + {article => } + , + ) + await waitFor(() => { + expect(getByTestId(container, 'title').textContent).toBe('Hello World') + }) + expect(getByTestId(container, 'heading').textContent).toBe('translated:heading') + expect(useCalls).toBeGreaterThan(0) + }) + + test('hooks work inside use()', async () => { + const TranslatorContext = createContext<(key: string) => string>(() => 'missing-provider') + + const Comp = createComponent() + .entity('article', schema.Article) + .use(() => ({ t: useContext(TranslatorContext) })) + .render(({ article, t }) => ( +
+

{t('heading')}

+ +
+ )) + + const { container } = renderWithBindx( + `ctx:${key}`}> + + {article => } + + , + ) + + await waitFor(() => { + expect(getByTestId(container, 'title').textContent).toBe('Hello World') + }) + expect(getByTestId(container, 'heading').textContent).toBe('ctx:heading') + }) + + test('chained use() functions see values of earlier ones', async () => { + const Comp = createComponent() + .entity('article', schema.Article) + .use(() => ({ base: 'A' })) + .use(({ base }) => ({ derived: `${base}B` })) + .render(({ article, derived }) => ( +
+ {derived} + +
+ )) + + const { container } = renderWithBindx( + + {article => } + , + ) + + await waitFor(() => { + expect(getByTestId(container, 'title').textContent).toBe('Hello World') + }) + expect(getByTestId(container, 'derived').textContent).toBe('AB') + }) + + test('use() values are not part of public props (parent does not pass them)', async () => { + // The component itself provides `t`; the call site only passes the entity. + const Comp = createComponent() + .entity('article', schema.Article) + .use(() => ({ t: (key: string): string => `own:${key}` })) + .render(({ article, t }) => ( + {t('x')} + )) + + const { container } = renderWithBindx( + + {article => } + , + ) + + await waitFor(() => { + expect(getByTestId(container, 'text').textContent).toBe('own:xHello World') + }) + }) +}) + +describe('selection analysis robustness', () => { + test('a component crashing during analysis does not cost siblings their selection', async () => { + // The tolerant scalar stand-in cannot save indexing a REAL object with a + // mocked key: LABELS[''] is undefined and `.x` throws during analysis. + const LABELS: Record = { k1: { x: 'label-one' } } + + const Broken = createComponent() + .entity('article', schema.Article) + .props<{ labelKey: string }>() + .render(({ labelKey }) => ( + {LABELS[labelKey]!.x} + )) + + const Good = createComponent() + .entity('article', schema.Article) + .render(({ article }) => ( + + )) + + const { container } = renderWithBindx( + + {article => ( +
+ + +
+ )} + , + ) + + // Good sibling's field must be fetched despite Broken's analysis crash; + // Broken itself renders fine at runtime (real labelKey exists). + await waitFor(() => { + expect(getByTestId(container, 'good').textContent).toBe('Hello World') + }) + expect(getByTestId(container, 'broken').textContent).toBe('label-one') + }) + + test('fields accessed before an analysis crash are still collected', () => { + const REAL: Record = {} + + const Comp = createComponent() + .entity('article', schema.Article) + .props<{ k: string }>() + .render(({ article, k }) => ( +
+ + {REAL[k]!.toUpperCase()} +
+ )) + + const selection = getComponentSelection(Comp, 'article') + expect(selection).toBeDefined() + expect([...selection!.fields.keys()]).toContain('title') + }) +})