Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions .changeset/fix-journey-mutation.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
---
'@data-client/normalizr': patch
'@data-client/core': patch
'@data-client/endpoint': patch
'@data-client/rest': patch
'@data-client/graphql': patch
'@data-client/react': patch
---

Fix cached journey being mutated on repeated result-cache hits.

`GlobalCache.getResults` called `paths.shift()` on a cache hit, mutating
the `journey` array stored by reference on the `WeakDependencyMap` `Link`
node. After the first hit stripped the placeholder input slot, every
subsequent hit on the same cached entry would shift off a real
`EntityPath`, progressively losing subscription entries. This could cause
missed `countRef` tracking (premature GC of still-referenced entities)
and incorrect `entityExpiresAt` calculations. The hit path now returns a
non-mutating copy.
6 changes: 6 additions & 0 deletions .changeset/peerdeps-017.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@data-client/img': patch
'@data-client/test': patch
---

Bump `@data-client/react` peer dependency range to include `^0.17.0`.
93 changes: 93 additions & 0 deletions packages/core/src/controller/__tests__/getResponseMeta-countRef.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
import { Endpoint, Entity } from '@data-client/endpoint';

import { GCPolicy } from '../../state/GCPolicy';
import { initialState } from '../../state/reducer/createReducer';
import Controller from '../Controller';

/** Integration test for the `paths` → `countRef` pipeline.
*
* React hooks (`useSuspense`, `useCache`, `useLive`, `useDLE`, `useQuery`)
* all call `controller.getResponseMeta(...)` during `useMemo` and then
* pass the returned `countRef` to `useEffect`. `useEffect` executes
* `countRef` once per hook instance — iterating the **captured** `paths`
* array to increment `GCPolicy.entityCount` per referenced entity. When
* the component unmounts (or `data` changes) the returned `decrement`
* closure iterates that same captured `paths` array to decrement.
*
* Before the fix, `globalCache.getResults` called `paths.shift()` on a
* result-cache hit, mutating the journey array stored by reference on
* `WeakDependencyMap.Link.journey`. Because `paths` aliases that stored
* journey, every successive hit dropped one more real `EntityPath` from
* the subscription list. The Nth component mount (for N ≥ 3, same
* endpoint + args, entities unchanged) would have its `countRef`
* iterate a progressively shorter list — skipping the first real
* entity, then the second, etc. — leaving `entityCount` under-counted
* for those entities and allowing GC to reap entities that mounted
* components still subscribe to.
*/
describe('Controller.getResponseMeta + GCPolicy.countRef integration', () => {
it('entityCount tracks all entities for each subscriber across repeated result-cache hits', () => {
class Foo extends Entity {
id = '';
pk() {
return this.id;
}
}
const ep = new Endpoint(() => Promise.resolve(), {
key: () => 'listFoo',
schema: [Foo],
});

const gcPolicy = new GCPolicy();
const controller = new Controller({ gcPolicy });
gcPolicy.init(controller);

const state = {
...initialState,
entities: {
Foo: {
'1': { id: '1' },
'2': { id: '2' },
},
},
endpoints: {
[ep.key()]: ['1', '2'],
},
};

// Three successive `getResponseMeta` calls simulate three React
// components mounting with the same endpoint + args, where the store
// has not changed between renders. Call 1 is a result-cache miss;
// calls 2 and 3 are hits — each hit mutates the shared journey.
const m1 = controller.getResponseMeta(ep, state);
const m2 = controller.getResponseMeta(ep, state);
const m3 = controller.getResponseMeta(ep, state);

// Simulate `useEffect(countRef, [data])` firing for each mount.
// Pre-fix: m2.paths and m3.paths both aliased the shared journey,
// and by the time the countRefs run the journey has been shifted
// twice — so both iterate `[Foo|'2']` only, silently skipping
// Foo|'1'. Only m1 (miss path, fresh `paths`) counted Foo|'1'.
const dec1 = m1.countRef();
const dec2 = m2.countRef();
const dec3 = m3.countRef();

// Each mount must have incremented *both* entities.
// Pre-fix: Foo|'1' = 1 (only m1 counted it); Foo|'2' = 3.
expect(gcPolicy['entityCount'].get('Foo')?.get('1')).toBe(3);
expect(gcPolicy['entityCount'].get('Foo')?.get('2')).toBe(3);

// Simulate all three components unmounting (or `data` changing).
// Pre-fix: only dec1 would decrement Foo|'1' (taking it from 1 to
// 0) — observable as premature GC-eligibility of Foo|'1' while
// the other two subscribers still depended on it.
dec1();
dec2();
dec3();

expect(gcPolicy['entityCount'].get('Foo')?.get('1')).toBeUndefined();
expect(gcPolicy['entityCount'].get('Foo')?.get('2')).toBeUndefined();

gcPolicy.cleanup();
});
});
88 changes: 88 additions & 0 deletions packages/core/src/controller/__tests__/getResponseMeta-paths.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
import { Endpoint, Entity } from '@data-client/endpoint';

import { initialState } from '../../state/reducer/createReducer';
import Controller from '../Controller';

/** Integration test: `Controller.getResponseMeta()` is the seam every read
* hook (`useSuspense`, `useCache`, `useFetch`, `useDLE`, `useLive`) calls.
* Multiple subscribers to the same endpoint + args + entities must observe
* the same `expiresAt`, otherwise late subscribers silently skip the
* entity-expiry refetch in `useSuspense`'s gate
* `if (Date.now() <= expiresAt && !forceFetch) return;`.
*
* The bug fixed by this PR was a `paths.shift()` mutation on the journey
* stored by reference on `WeakDependencyMap.Link.journey`. Each successive
* cache hit returned a journey one entry shorter than the last. The first
* entity dropped is the one with the *earliest* expiry, so subscriber 2's
* `entityExpiresAt(paths, …)` skipped it and computed a too-late
* `expiresAt`; subscriber 3 saw an empty array and got `Infinity`.
*
* The path is GC-independent: it fires under `ImmortalGCPolicy` too, since
* `entityExpiresAt` is unconditional whenever the endpoint has no
* top-level `meta.expiresAt` (the common case for state populated via
* `controller.set(Entity, …)`, SSR hydration, or `useQuery`).
*/
describe('Controller.getResponseMeta repeated calls', () => {
it('returns identical expiresAt across subscribers (journey is not consumed by reads)', () => {
class Foo extends Entity {
id = '';
pk() {
return this.id;
}
}
const ep = new Endpoint(() => Promise.resolve(), {
key: () => 'listFoo',
schema: [Foo],
});

const controller = new Controller({});

// Endpoint slot exists (so we hit the `denormalize` path) but the
// endpoint itself has no top-level `meta.expiresAt` yet — mirrors
// apps that hydrate from SSR or use `controller.set(Entity, …)`
// without going through an endpoint fetch. In that shape,
// `Controller.getResponseMeta()` falls back to walking per-entity
// meta via `entityExpiresAt(paths, …)`. Foo|'1' has the earliest
// expiry — the first journey entry the buggy `paths.shift()` would
// drop.
const FOO_1_EXPIRY = 100;
const FOO_2_EXPIRY = 1_000_000;
const state = {
...initialState,
entities: {
Foo: {
'1': { id: '1' },
'2': { id: '2' },
},
},
entitiesMeta: {
Foo: {
'1': { date: 0, fetchedAt: 0, expiresAt: FOO_1_EXPIRY },
'2': { date: 0, fetchedAt: 0, expiresAt: FOO_2_EXPIRY },
},
},
endpoints: {
[ep.key()]: ['1', '2'],
},
};

// Three successive calls simulate three React components mounting
// with the same endpoint + args. Call 1 is a result-cache miss;
// calls 2 and 3 are hits.
const m1 = controller.getResponseMeta(ep, state);
const m2 = controller.getResponseMeta(ep, state);
const m3 = controller.getResponseMeta(ep, state);

// Subscriber 1 picks the earliest entity expiry. Subscribers 2+ must
// pick the same one — pre-fix they observed FOO_2_EXPIRY (m2) and
// Infinity (m3), silently suppressing entity-expiry refetch.
expect(m1.expiresAt).toBe(FOO_1_EXPIRY);
expect(m2.expiresAt).toBe(m1.expiresAt);
expect(m3.expiresAt).toBe(m1.expiresAt);

// Cached data must remain referentially equal across hits — the
// memo contract `useSuspense` etc. rely on for skip-rerender.
expect(m2.data).toBe(m1.data);
expect(m3.data).toBe(m1.data);
});
});
2 changes: 1 addition & 1 deletion packages/img/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@
"@data-client/endpoint": "workspace:^"
},
"peerDependencies": {
"@data-client/react": "^0.1.0 || ^0.2.0 || ^0.3.0 || ^0.4.0 || ^0.5.0 || ^0.7.0 || ^0.8.0 || ^0.9.0 || ^0.10.0 || ^0.11.0 || ^0.12.0 || ^0.13.0 || ^0.14.0 || ^0.15.0 || ^0.16.0",
"@data-client/react": "^0.1.0 || ^0.2.0 || ^0.3.0 || ^0.4.0 || ^0.5.0 || ^0.7.0 || ^0.8.0 || ^0.9.0 || ^0.10.0 || ^0.11.0 || ^0.12.0 || ^0.13.0 || ^0.14.0 || ^0.15.0 || ^0.16.0 || ^0.17.0",
"@types/react": "^16.14.0 || ^17.0.0 || ^18.0.0-0 || ^19.0.0",
"react": "^16.14.0 || ^17.0.0 || ^18.0.0-0 || ^19.0.0"
},
Expand Down
41 changes: 41 additions & 0 deletions packages/normalizr/src/__tests__/globalCache.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,47 @@ describe('GlobalCache', () => {
expect(r2.paths).toEqual(r1.paths);
});

it('does not mutate cached journey across repeated result-cache hits', () => {
// Regression: on a resultCache hit, `getResults` must strip the
// placeholder input slot from the returned paths WITHOUT mutating the
// journey array stored on the WeakDependencyMap `Link` node. A mutating
// `paths.shift()` progressively removes real entity paths from the
// stored journey on each successive hit, corrupting the subscription
// list (missed countRef, incorrect entityExpiresAt).
const entities = {
Foo: { '1': { id: '1' }, '2': { id: '2' } },
};
const { getEntity, getCache, resultCache } = makeDeps(entities);
const input = [{ id: '1' }, { id: '2' }];

const frame1 = new GlobalCache(getEntity, getCache, resultCache, []);
const r1 = frame1.getResults(input, true, () => {
frame1.getEntity('1', Foo, entities.Foo['1'], m =>
m.set('1', { ...entities.Foo['1'] }),
);
frame1.getEntity('2', Foo, entities.Foo['2'], m =>
m.set('2', { ...entities.Foo['2'] }),
);
return [{ id: '1' }, { id: '2' }];
});
expect(r1.paths).toEqual([
{ key: 'Foo', pk: '1' },
{ key: 'Foo', pk: '2' },
]);

// Repeated hits must each return the full set of entity paths.
for (let n = 0; n < 3; n++) {
const frame = new GlobalCache(getEntity, getCache, resultCache, []);
const { paths } = frame.getResults(input, true, () => {
throw new Error('resultCache must hit');
});
expect(paths).toEqual([
{ key: 'Foo', pk: '1' },
{ key: 'Foo', pk: '2' },
]);
}
});

it('cache hit across frames also works when argsKey was used (paths filtered)', () => {
// Once the resultCache has stored any function-typed dep, future hits
// must strip them when returning paths. Ensures the on-hit filter at
Expand Down
15 changes: 13 additions & 2 deletions packages/normalizr/src/memo/WeakDependencyMap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,18 @@ export default class WeakDependencyMap<
return [curLink.value, curLink.journey] as readonly [V, Path[]];
}

set(dependencies: Dep<Path, K>[], value: V, args: readonly any[] = []) {
set(
dependencies: Dep<Path, K>[],
value: V,
args: readonly any[] = [],
/** Optional consumer-facing journey returned to `get()` callers verbatim.
* Defaults to `dependencies.map(d => d.path)`. Pass an explicit array to
* skip the per-write `.map(...)` and (more importantly) to skip per-hit
* post-processing — see `GlobalCache.getResults` for the read-side
* payoff. The array becomes a shared reference held by every subsequent
* cache hit; callers MUST NOT mutate it. */
journey?: Path[],
) {
if (dependencies.length < 1) throw new KeySize();
let curLink: Link<Path, K, V> = this as any;
for (const dep of dependencies) {
Expand Down Expand Up @@ -98,7 +109,7 @@ export default class WeakDependencyMap<
curLink.nextPath = undefined;
curLink.value = value;
// we could recompute this on get, but it would have a cost and we optimize for `get`
curLink.journey = dependencies.map(d => d.path) as Path[];
curLink.journey = journey ?? (dependencies.map(d => d.path) as Path[]);
}

/** True once any `argsKey`-style dep has been written. Consumers can use
Expand Down
26 changes: 11 additions & 15 deletions packages/normalizr/src/memo/globalCache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -173,24 +173,20 @@ export default class GlobalCache implements Cache {

if (paths === undefined) {
data = computeValue();
// we want to do this before we fill our 'input' entry
// build the consumer-facing subscription list once, here.
// `paths()` already excludes the input placeholder slot and (when
// `_hasArgsKey`) function-typed `argsKey` deps, so it's exactly the
// shape every hit-branch consumer needs. We hand it to the cache as
// the journey so subsequent hits return it by reference — no per-hit
// `slice(1)` or filter pass required.
paths = this.paths();
// fill pre-allocated slot 0 with the input reference
// fill pre-allocated slot 0 with the input reference (chain key only)
this.dependencies[0] = { path: { key: '', pk: '' }, entity: input };
this._resultCache.set(this.dependencies, data, this._args);
} else {
paths.shift();
// strip any function-typed (`argsKey`) paths — not subscribable entities.
// Only possible when the result cache has ever stored such a dep.
if (this._resultCache.hasStringDeps) {
for (let i = 0; i < paths.length; i++) {
if (typeof paths[i] === 'function') {
paths = paths.filter(p => typeof p !== 'function') as EntityPath[];
break;
}
}
}
this._resultCache.set(this.dependencies, data, this._args, paths);
}
// hit branch: `paths` aliases the stored Link.journey — return as-is.
// Callers must not mutate it (entityExpiresAt and createCountRef both
// read-only). The journey-mutation regression test guards this contract.
return { data, paths };
}

Expand Down
2 changes: 1 addition & 1 deletion packages/test/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@
"@testing-library/react": "^16.0.0"
},
"peerDependencies": {
"@data-client/react": "^0.12.15 || ^0.13.0 || ^0.14.0 || ^0.15.0 || ^0.16.0",
"@data-client/react": "^0.12.15 || ^0.13.0 || ^0.14.0 || ^0.15.0 || ^0.16.0 || ^0.17.0",
"@testing-library/react-hooks": "^8.0.0",
"@testing-library/react-native": "^13.0.0",
"@types/react": "^16.14.0 || ^17.0.0 || ^18.0.0 || ^19.0.0-0 || ^19.0.0",
Expand Down
4 changes: 2 additions & 2 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3262,7 +3262,7 @@ __metadata:
react-dom: "npm:^19.2.1"
rollup-plugins: "workspace:*"
peerDependencies:
"@data-client/react": ^0.1.0 || ^0.2.0 || ^0.3.0 || ^0.4.0 || ^0.5.0 || ^0.7.0 || ^0.8.0 || ^0.9.0 || ^0.10.0 || ^0.11.0 || ^0.12.0 || ^0.13.0 || ^0.14.0 || ^0.15.0 || ^0.16.0
"@data-client/react": ^0.1.0 || ^0.2.0 || ^0.3.0 || ^0.4.0 || ^0.5.0 || ^0.7.0 || ^0.8.0 || ^0.9.0 || ^0.10.0 || ^0.11.0 || ^0.12.0 || ^0.13.0 || ^0.14.0 || ^0.15.0 || ^0.16.0 || ^0.17.0
"@types/react": ^16.14.0 || ^17.0.0 || ^18.0.0-0 || ^19.0.0
react: ^16.14.0 || ^17.0.0 || ^18.0.0-0 || ^19.0.0
peerDependenciesMeta:
Expand Down Expand Up @@ -3369,7 +3369,7 @@ __metadata:
react-test-renderer: "npm:*"
rollup-plugins: "workspace:*"
peerDependencies:
"@data-client/react": ^0.12.15 || ^0.13.0 || ^0.14.0 || ^0.15.0 || ^0.16.0
"@data-client/react": ^0.12.15 || ^0.13.0 || ^0.14.0 || ^0.15.0 || ^0.16.0 || ^0.17.0
"@testing-library/react-hooks": ^8.0.0
"@testing-library/react-native": ^13.0.0
"@types/react": ^16.14.0 || ^17.0.0 || ^18.0.0 || ^19.0.0-0 || ^19.0.0
Expand Down
Loading