diff --git a/packages/bindx-react/src/hooks/useRootSelection.ts b/packages/bindx-react/src/hooks/useRootSelection.ts index 4afccf92..06399278 100644 --- a/packages/bindx-react/src/hooks/useRootSelection.ts +++ b/packages/bindx-react/src/hooks/useRootSelection.ts @@ -80,6 +80,10 @@ export function useRootSelection(params: UseRootSelectionParams): SelectionColle const runtimeResult = useSelectionCollection({ entityType, depsKey: isWalkSkipped ? `${depsKey}::compiled` : depsKey, + source: isWalkSkipped ? undefined : children, + // With a compiled selection the walk only feeds the validate-mode diff, which must + // compare against what these children read, not a union with earlier ones. + grow: compiledResult === null, collect: collector => { if (isWalkSkipped) { return null diff --git a/packages/bindx-react/src/hooks/useSelectionCollection.ts b/packages/bindx-react/src/hooks/useSelectionCollection.ts index 5aab2f1f..048e0565 100644 --- a/packages/bindx-react/src/hooks/useSelectionCollection.ts +++ b/packages/bindx-react/src/hooks/useSelectionCollection.ts @@ -3,7 +3,7 @@ import { createCollectorProxy } from '../jsx/proxy.js' import { collectSelection, debugSelection } from '../jsx/analyzer.js' import { mergeSelections } from '../jsx/SelectionMeta.js' import { buildQueryFromSelection, SelectionScope } from '@contember/bindx' -import type { SelectionMeta, SchemaRegistry } from '@contember/bindx' +import type { SelectionMeta, SelectionFieldMeta, SchemaRegistry } from '@contember/bindx' import type { EntityAccessor } from '../jsx/types.js' import { BindxContext } from './BackendAdapterContext.js' @@ -17,6 +17,22 @@ export interface UseSelectionCollectionOptions { depsKey: string /** Collector function — receives a collector proxy, returns JSX for analysis */ collect: (collector: EntityAccessor) => ReactNode + /** + * The user-supplied render function `collect` wraps. A new identity re-runs the + * collection: React reuses an across conditional branches that differ only + * in their children, and those children may read fields the previous ones did not. + * For one entity type and `depsKey` the selection only grows — fields the new children + * no longer read stay selected — so the result changes (and the data is refetched) only + * when a field nobody requested before appears. Pass a stable `source` from a parent + * that re-renders often: every new identity runs the collection again. + */ + source?: unknown + /** + * Whether the selection grows across collections (default `true`). Off, every + * collection stands on its own — for a caller that fetches a different selection and + * only compares against this one. + */ + grow?: boolean /** Extra data to include in the query key (e.g., filter/orderBy for lists) */ queryKeyExtra?: Record } @@ -37,6 +53,72 @@ export interface SelectionCollectionResult { interface SelectionCache { selection: SelectionMeta queryKey: string + /** What the selection was grown under; a change in any of them starts afresh. */ + entityType: string + depsKey: string + schemaRegistry: unknown +} + +/** Deep copy, so a merge into it cannot reach selections other code still holds. */ +function cloneSelection(selection: SelectionMeta): SelectionMeta { + const fields = new Map() + for (const [key, field] of selection.fields) { + fields.set(key, field.nested ? { ...field, nested: cloneSelection(field.nested) } : { ...field }) + } + return { fields } +} + +/** + * Drops from `target` the has-many entries that `collected` replaces. `` gives + * each filter / orderBy / limit / offset its own alias, so a field the collected children + * read under parameters `target` has not seen yet is a replacement: the cached + * parameterised aliases of that field that the children no longer read go, and a search + * or a page held in parent state swaps its subquery instead of piling up every earlier + * one. Without such a new alias nothing is dropped, so a hidden filtered list survives + * growth elsewhere. Recurses into relations both sides read. + */ +function dropReplacedHasManyAliases(target: SelectionMeta, collected: SelectionMeta): void { + const replacedFieldNames = new Set() + for (const [key, field] of collected.fields) { + if (field.isArray && field.hasManyParams && key !== field.fieldName && !target.fields.has(key)) { + replacedFieldNames.add(field.fieldName) + } + } + for (const [key, field] of target.fields) { + const collectedField = collected.fields.get(key) + if (collectedField) { + if (field.nested && collectedField.nested) { + dropReplacedHasManyAliases(field.nested, collectedField.nested) + } + continue + } + if (field.isArray && key !== field.fieldName && replacedFieldNames.has(field.fieldName)) { + target.fields.delete(key) + } + } +} + +/** + * Whether every field `selection` requests is already requested by `covering`, + * recursively, with the same relation kind and has-many parameters. + */ +function isSelectionCovered(selection: SelectionMeta, covering: SelectionMeta): boolean { + for (const [key, field] of selection.fields) { + const existing = covering.fields.get(key) + if (!existing || existing.fieldName !== field.fieldName) { + return false + } + if ((field.isRelation && !existing.isRelation) || (field.isArray && !existing.isArray)) { + return false + } + if (field.hasManyParams && JSON.stringify(field.hasManyParams) !== JSON.stringify(existing.hasManyParams)) { + return false + } + if (field.nested && !(existing.nested && isSelectionCovered(field.nested, existing.nested))) { + return false + } + } + return true } /** @@ -47,19 +129,29 @@ interface SelectionCache { * 3. Analyzes returned JSX for component-level selections * 4. Merges both sources of selection * - * **Limitation:** Selection is collected once per `depsKey`. - * Fields accessed conditionally in the collect function are only captured - * if they are accessed during the initial collection phase. + * Selection is collected once per `depsKey` and `source` and, unless `grow` is off, + * only grows for one entity type, `depsKey` and schema registry; a has-many read under + * new parameters replaces its earlier subquery. Re-renders driven by this entity's own + * store subscription keep the same `source`, so they do not re-run the collection. An + * nested in another entity's children gets a new inline `children` whenever the + * outer one re-renders — on every store change it subscribes to — and collects again each + * time; the result stays cached, so there is no refetch, only the walk. + * + * A compiled selection (`useRootSelection`) does not grow: it is replaced on a branch + * switch and can shrink. + * + * **Limitation:** Fields accessed conditionally in the collect function are only + * captured if they are accessed when the collection runs. * * @internal This hook is for internal use only. */ export function useSelectionCollection( options: UseSelectionCollectionOptions, ): SelectionCollectionResult { - const { entityType, depsKey, collect, queryKeyExtra } = options + const { entityType, depsKey, collect, source, grow = true, queryKeyExtra } = options const bindxContext = useContext(BindxContext) - // Stable collect ref - we use a ref to avoid re-running useMemo on every render + // `collect` is usually an inline closure; `source` decides when collection re-runs. const collectRef = useRef(collect) collectRef.current = collect @@ -79,16 +171,43 @@ export function useSelectionCollection( // Analyze the returned JSX for component-level selections let jsxSel: SelectionMeta + let jsxAnalysed = true try { jsxSel = collectSelection(jsx) } catch { // staticRender of nested components may crash during collection jsxSel = { fields: new Map() } + jsxAnalysed = false } // Convert scope to SelectionMeta and merge with JSX selection - const selection = scope.toSelectionMeta() - mergeSelections(selection, jsxSel) + const collected = scope.toSelectionMeta() + mergeSelections(collected, jsxSel) + + // Grow, never shrink, for one entity type and depsKey: children that stop reading a + // field (a hidden section, a switched branch) must not trigger a refetch, and coming + // back to them must find the field still fetched. A has-many read under new + // parameters replaces its earlier subquery rather than adding to it. + const cache = selectionCacheRef.current + const growsCache = grow + && cache !== null + && cache.entityType === entityType + && cache.depsKey === depsKey + && cache.schemaRegistry === schemaRegistry + if (growsCache && isSelectionCovered(collected, cache.selection)) { + return cache + } + // The collected selection can share nested objects with component selections + // and the cached one with useEntity, so the union is built in a fresh copy. + const selection = growsCache ? cloneSelection(cache.selection) : collected + if (growsCache) { + // A failed JSX analysis misses the aliased has-manys the children render, so + // nothing cached may be treated as replaced. + if (jsxAnalysed) { + dropReplacedHasManyAliases(selection, collected) + } + mergeSelections(selection, collected) + } // Debug output can be enabled via the debug prop on BindxProvider if (bindxContext?.debug) { @@ -103,20 +222,29 @@ export function useSelectionCollection( : JSON.stringify(query) // If the selection hasn't actually changed, return cached values - const cache = selectionCacheRef.current if (cache && cache.queryKey === newQueryKey) { - return cache + if (cache.entityType === entityType && cache.depsKey === depsKey && cache.schemaRegistry === schemaRegistry) { + return cache + } + // Same query under another entity, depsKey or registry: keep the selection and + // record what it belongs to now, in a new object so the previous result stays intact. + const rekeyed: SelectionCache = { ...cache, entityType, depsKey, schemaRegistry } + selectionCacheRef.current = rekeyed + return rekeyed } // Update cache with new values const newCache: SelectionCache = { selection, queryKey: newQueryKey, + entityType, + depsKey, + schemaRegistry, } selectionCacheRef.current = newCache return newCache - }, [entityType, depsKey, schemaRegistry]) // Only depend on entity identity, deps key, and schema + }, [entityType, depsKey, source, grow, schemaRegistry]) return result } diff --git a/packages/bindx-react/src/jsx/components/EntityList.tsx b/packages/bindx-react/src/jsx/components/EntityList.tsx index a87f8011..5fb887ea 100644 --- a/packages/bindx-react/src/jsx/components/EntityList.tsx +++ b/packages/bindx-react/src/jsx/components/EntityList.tsx @@ -38,7 +38,8 @@ export interface EntityListProps = Recor /** * EntityList component - orchestrates the two-pass rendering approach for lists. * - * Phase 1 (Collection): Renders children once with collector proxy to determine which fields are needed + * Phase 1 (Collection): Renders children with a collector proxy to determine which fields are needed — + * again whenever the children function changes, growing the selection for the same entity and filter/order/page * Phase 2 (Loading): Fetches data based on collected selection * Phase 3 (Runtime): Renders children for each item with real data accessors * @@ -79,6 +80,7 @@ function EntityListComponent>({ const { selection, queryKey: selectionQueryKey } = useSelectionCollection({ entityType, depsKey: optionsKey, + source: children, collect: collector => children(collector as unknown as EntityAccessor>, 0), queryKeyExtra: { filter, orderBy, limit, offset }, }) diff --git a/tests/react/jsx/entityBranchSwitchSelection.test.tsx b/tests/react/jsx/entityBranchSwitchSelection.test.tsx new file mode 100644 index 00000000..9e63a175 --- /dev/null +++ b/tests/react/jsx/entityBranchSwitchSelection.test.tsx @@ -0,0 +1,452 @@ +// Regression test for https://github.com/contember/bindx/issues/154 +import '../../setup' +import { describe, test, expect, afterEach } from 'bun:test' +import { render, waitFor, cleanup, act } from '@testing-library/react' +import React, { useState } from 'react' +import { BindxProvider, MockAdapter, Entity, EntityList, Field, HasMany } from '@contember/bindx-react' +import { getByTestId, queryByTestId, createMockData, schema, testSchema } from '../../shared' + +afterEach(() => { + cleanup() +}) + +describe(' reused across conditional branches', () => { + test('should collect the new children selection when the branch switches but entity and by stay the same', async () => { + const adapter = new MockAdapter(createMockData(), { delay: 0 }) + + let setTab: (tab: 'title' | 'content') => void = () => {} + + // Two tabs render an for the same record at the same position in the + // tree, with different children. React reuses the instance; only `children` changes. + function App() { + const [tab, setLocalTab] = useState<'title' | 'content'>('title') + setTab = setLocalTab + + return tab === 'title' + ? ( + + {article =>
} + + ) + : ( + + {article =>
} + + ) + } + + const { container } = render( + + + , + ) + + await waitFor(() => { + expect(queryByTestId(container, 'title')).not.toBeNull() + }) + + act(() => setTab('content')) + + await waitFor(() => { + expect(queryByTestId(container, 'content')).not.toBeNull() + }) + expect(getByTestId(container, 'content').textContent).toBe('This is the content') + }) + + test('should collect the new children selection when an branch switches with the same options', async () => { + const adapter = new MockAdapter(createMockData(), { delay: 0 }) + + let setTab: (tab: 'title' | 'content') => void = () => {} + + function App() { + const [tab, setLocalTab] = useState<'title' | 'content'>('title') + setTab = setLocalTab + + return tab === 'title' + ? ( + + {article =>
} + + ) + : ( + + {article =>
} + + ) + } + + const { container } = render( + + + , + ) + + await waitFor(() => { + expect(queryByTestId(container, 'title-article-1')).not.toBeNull() + }) + + act(() => setTab('content')) + + await waitFor(() => { + expect(queryByTestId(container, 'content-article-1')).not.toBeNull() + }) + expect(getByTestId(container, 'content-article-1').textContent).toBe('This is the content') + }) + + test('should not refetch when children stop reading a field or come back to it', async () => { + const adapter = new MockAdapter(createMockData(), { delay: 0 }) + const query = adapter.query.bind(adapter) + let queryCount = 0 + adapter.query = (...args) => { + queryCount++ + return query(...args) + } + + let setShowContent: (show: boolean) => void = () => {} + + function App() { + const [showContent, setLocalShowContent] = useState(true) + setShowContent = setLocalShowContent + + return ( + + {article => ( +
+
+ {showContent &&
} +
+ )} + + ) + } + + const { container } = render( + + + , + ) + + await waitFor(() => { + expect(queryByTestId(container, 'content')).not.toBeNull() + }) + const queriesAfterLoad = queryCount + + act(() => setShowContent(false)) + act(() => setShowContent(true)) + act(() => setShowContent(false)) + act(() => setShowContent(true)) + await new Promise(resolve => setTimeout(resolve, 20)) + + expect(queryCount).toBe(queriesAfterLoad) + expect(getByTestId(container, 'content').textContent).toBe('This is the content') + }) + + test('should keep the selection and skip refetching when the parent re-renders with the same fields', async () => { + const adapter = new MockAdapter(createMockData(), { delay: 0 }) + const query = adapter.query.bind(adapter) + let queryCount = 0 + adapter.query = (...args) => { + queryCount++ + return query(...args) + } + + let rerender: () => void = () => {} + + function App() { + const [, setTick] = useState(0) + rerender = () => setTick(tick => tick + 1) + + return ( + + {article =>
} + + ) + } + + const { container } = render( + + + , + ) + + await waitFor(() => { + expect(queryByTestId(container, 'title')).not.toBeNull() + }) + const queriesAfterLoad = queryCount + + act(() => rerender()) + act(() => rerender()) + await new Promise(resolve => setTimeout(resolve, 20)) + + expect(queryCount).toBe(queriesAfterLoad) + expect(getByTestId(container, 'title').textContent).toBe('Hello World') + }) + + test('should not carry fields of the previous entity into an reused for another entity', async () => { + const adapter = new MockAdapter(createMockData(), { delay: 0 }) + const query = adapter.query.bind(adapter) + const requestedFields: Record = {} + adapter.query = (queries, options) => { + for (const entry of queries) { + const fields = 'spec' in entry ? entry.spec.fields.map(field => field.sourcePath[0] ?? field.name) : [] + requestedFields[entry.entityType] = fields + } + return query(queries, options) + } + + let setTab: (tab: 'articles' | 'authors') => void = () => {} + + function App() { + const [tab, setLocalTab] = useState<'articles' | 'authors'>('articles') + setTab = setLocalTab + + return tab === 'articles' + ? ( + + {article =>
} + + ) + : ( + + {author =>
} + + ) + } + + const { container } = render( + + + , + ) + + await waitFor(() => { + expect(queryByTestId(container, 'article-article-1')).not.toBeNull() + }) + + act(() => setTab('authors')) + + await waitFor(() => { + expect(queryByTestId(container, 'author-author-1')).not.toBeNull() + }) + expect(getByTestId(container, 'author-author-1').textContent).toBe('John Doe') + expect(requestedFields['Author']).not.toContain('title') + }) + + test('should start the selection afresh when switches to another record', async () => { + const adapter = new MockAdapter(createMockData(), { delay: 0 }) + const query = adapter.query.bind(adapter) + const requestedFields: Record = {} + adapter.query = (queries, options) => { + for (const entry of queries) { + if (entry.type === 'get') { + requestedFields[JSON.stringify(entry.by)] = entry.spec.fields.map(field => field.sourcePath[0] ?? field.name) + } + } + return query(queries, options) + } + + let setArticleId: (id: string) => void = () => {} + + function App() { + const [articleId, setLocalArticleId] = useState('article-1') + setArticleId = setLocalArticleId + + return articleId === 'article-1' + ? ( + + {article => ( +
+ + +
+ )} + + ) + : ( + + {article =>
} + + ) + } + + const { container } = render( + + + , + ) + + await waitFor(() => { + expect(getByTestId(container, 'article').textContent).toBe('Hello WorldThis is the content') + }) + + act(() => setArticleId('article-2')) + + await waitFor(() => { + expect(getByTestId(container, 'article').textContent).toBe('Another Article') + }) + expect(requestedFields[JSON.stringify({ id: 'article-2' })]).not.toContain('content') + }) + + test('should replace a has-many subquery when its filter changes instead of adding another', async () => { + const adapter = new MockAdapter(createMockData(), { delay: 0 }) + const query = adapter.query.bind(adapter) + const tagSubqueries: number[] = [] + let lastTagFilters: unknown[] = [] + adapter.query = (queries, options) => { + for (const entry of queries) { + if (entry.type === 'get') { + const tagFields = entry.spec.fields.filter(field => field.sourcePath[0] === 'tags') + tagSubqueries.push(tagFields.length) + lastTagFilters = tagFields.map(field => field.filter).filter(filter => filter !== undefined) + } + } + return query(queries, options) + } + + let setSearch: (search: string) => void = () => {} + + function App() { + const [search, setLocalSearch] = useState('Java') + setSearch = setLocalSearch + + return ( + + {article => ( +
+ + {tag => } + +
+ )} + + ) + } + + const { container } = render( + + + , + ) + + await waitFor(() => { + expect(queryByTestId(container, 'tags')).not.toBeNull() + }) + + for (const search of ['Re', 'Rea', 'React']) { + act(() => setSearch(search)) + await new Promise(resolve => setTimeout(resolve, 10)) + } + + expect(tagSubqueries.length).toBeGreaterThan(1) + expect(tagSubqueries.every(count => count === tagSubqueries[0])).toBe(true) + expect(lastTagFilters).toEqual([{ name: { containsCI: 'React' } }]) + }) + + test('should not refetch a nested when the outer entity re-renders on a store change', async () => { + const adapter = new MockAdapter(createMockData(), { delay: 0 }) + const query = adapter.query.bind(adapter) + let queryCount = 0 + adapter.query = (...args) => { + queryCount++ + return query(...args) + } + + let setOuterTitle: (title: string) => void = () => {} + + function App() { + return ( + + {article => { + setOuterTitle = title => article.title.setValue(title) + return ( +
+
+ + {inner =>
} + +
+ ) + }} + + ) + } + + const { container } = render( + + + , + ) + + await waitFor(() => { + expect(queryByTestId(container, 'inner')).not.toBeNull() + }) + const queriesAfterLoad = queryCount + + act(() => setOuterTitle('Edited')) + act(() => setOuterTitle('Edited again')) + await new Promise(resolve => setTimeout(resolve, 20)) + + expect(getByTestId(container, 'outer').textContent).toBe('Edited again') + expect(getByTestId(container, 'inner').textContent).toBe('More content here') + expect(queryCount).toBe(queriesAfterLoad) + }) + + test('should keep a hidden filtered has-many when the selection grows elsewhere', async () => { + const adapter = new MockAdapter(createMockData(), { delay: 0 }) + const query = adapter.query.bind(adapter) + let queryCount = 0 + adapter.query = (...args) => { + queryCount++ + return query(...args) + } + + let setShowFiltered: (show: boolean) => void = () => {} + let setShowTitle: (show: boolean) => void = () => {} + + function App() { + const [showFiltered, setLocalShowFiltered] = useState(true) + const [showTitle, setLocalShowTitle] = useState(false) + setShowFiltered = setLocalShowFiltered + setShowTitle = setLocalShowTitle + + return ( + + {article => ( +
+ {tag => } + {showFiltered && ( +
+ {tag => } +
+ )} + {showTitle &&
} +
+ )} + + ) + } + + const { container } = render( + + + , + ) + + await waitFor(() => { + expect(queryByTestId(container, 'filtered')).not.toBeNull() + }) + + act(() => setShowFiltered(false)) + act(() => setShowTitle(true)) + await waitFor(() => { + expect(getByTestId(container, 'title').textContent).toBe('Hello World') + }) + const queriesAfterGrowth = queryCount + + act(() => setShowFiltered(true)) + await new Promise(resolve => setTimeout(resolve, 20)) + + expect(queryCount).toBe(queriesAfterGrowth) + expect(getByTestId(container, 'filtered').textContent).not.toBe('') + }) +})