Add derived path dependencies - #4
Conversation
|
|
||
| // Normalize once so the same key drives both state lookup and notification. | ||
| const pathKey = normalizePath(path); | ||
| const derivedOwner = derivedByTarget.get(pathKey); |
There was a problem hiding this comment.
This only checks exact target writes. A reject-mode derived target can still be overwritten through an ancestor or descendant path, and the target subscribers may not be notified.
Repro:
const s = createBoltStore({ a: 1, node: { value: 0, other: 0 } });
s.derive("node.value", ["a"], ({ get }) => get("a") * 2);
let calls = 0;
s.subscribe("node.value", () => calls++);
s.set("node", { value: 99, other: 1 });
// node.value is now 99, calls is still 0Treat any write whose path overlaps a derived target as aimed at that target. Reject it by default, or if manualWrites is allowed, include the target path in changed paths and settle downstream nodes.
| const targetPathKey = normalizePath(targetPath); | ||
| const sourcePathKeys = [...new Set(sourcePaths.map((path) => normalizePath(path)))]; | ||
|
|
||
| if (derivedByTarget.has(targetPathKey)) { |
There was a problem hiding this comment.
This rejects only exact duplicate targets. Parent/child derived targets are still allowed, and they can corrupt each other because one derivation owns an object that contains the other derivation target.
Repro:
const s = createBoltStore({ x: 1, y: 2, a: { b: 0, c: 0 } });
s.derive("a", ["x"], ({ get }) => ({ b: get("x"), c: 0 }));
s.derive("a.b", ["y"], ({ get }) => get("y") * 10);
s.set("x", 2);
// a.b is now 2, not 20, and a.b subscribers were not notifiedUnless overlapping derived targets get a fully defined ownership model, registration should reject any new target that overlaps an existing derived target.
| ); | ||
| } | ||
|
|
||
| const didChange = writePathKey(pathKey, valueOrUpdater); |
There was a problem hiding this comment.
This writes the source path before derived settling, but there is no rollback if a derived compute throws. The store can mutate and then throw before any notification, leaving React subscribers stale.
Repro:
const s = createBoltStore({ a: 1, b: 2 });
let calls = 0;
s.subscribe("a", () => calls++);
s.derive("b", ["a"], ({ get }) => {
if (get("a") === 2) throw new Error("boom");
return get("a") * 2;
});
try { s.set("a", 2); } catch {}
// state.a is 2, state.b is stale, calls is 0Make the write transaction atomic around derived settling: save previous state and restore on compute/settle failure, then rethrow.
| continue; | ||
| } | ||
|
|
||
| affectedNodes.set(node.id, node); |
There was a problem hiding this comment.
Downstream nodes are queued as soon as an upstream target is discovered, before we know whether that upstream target actually changed. That makes no-op/equality-pass upstream nodes still run downstream computations. A downstream compute can throw or run side effects even though its declared source did not change.
Repro:
s.derive("b", ["a"], () => 0);
s.derive("c", ["b"], () => { throw new Error("should not run"); }, { initialize: false });
s.set("a", 2); // throws even though b stayed 0Settle incrementally: compute directly affected nodes first, and enqueue downstream dependents only when computeAndWriteDerivedNode() returns true.
| previous: unknown, | ||
| changedPathKeys: ReadonlySet<string>, | ||
| ) { | ||
| isComputingDerived = true; |
There was a problem hiding this comment.
This reentrancy guard is a single boolean, so it is not depth-safe. If a compute registers another initialized derivation, the nested compute sets the flag back to false while the outer compute is still running, and the outer compute can then call set().
Repro:
const s = createBoltStore({ a: 1, b: 0, c: 0, z: 0 });
s.derive("b", ["a"], () => {
s.derive("c", [], () => 1);
s.set("z", 1); // currently succeeds inside compute
return 2;
});Use a compute-depth counter or restore the previous flag value in finally. I would also reject derive() registration/disposal while a compute is active.
| for (let index = 0; index < queue.length; index += 1) { | ||
| const changedPathKey = queue[index]; | ||
|
|
||
| for (const node of derivedNodes.values()) { |
There was a problem hiding this comment.
The affected-node walk scans every derived node for every queued change, and the topo sort below scans the same node set again for every visit. Registration also revalidates the whole graph on every derive(). This turns derived chains into quadratic/cubic work and can freeze setup for large but plausible generated stores.
A local probe from the review thread showed chain registration around 22 ms for 100 nodes, 156 ms for 200, 1196 ms for 400, and a 2000-node chain did not finish within 60s.
Please add source/target adjacency indexes and incremental validation/traversal, or explicitly bound/document derived graph size. At minimum add a perf regression test for chained derivations so this does not ship unnoticed.
| TargetPath extends BoltPath<TState>, | ||
| >( | ||
| targetPath: TargetPath, | ||
| sourcePaths: readonly BoltRuntimePath[], |
There was a problem hiding this comment.
The target path is type checked, but source paths are only BoltRuntimePath[]. That is a type-safety hole in the main derived API: source typos compile and then the derived target silently never recomputes for the intended source.
Example:
store.derive("cart.total", ["cart.discont"], ({ get }) => get("cart.discount") ?? 0);
store.set("cart.discount", 5); // target does not recomputePrefer readonly BoltPath<TState>[] or a generic source tuple constrained to BoltPath<TState>. If dynamic untyped paths are intentional, make that an explicit escape hatch instead of the default typed API.
| * Nested changes notify root plus path prefixes. Root changes notify all | ||
| * listeners. One listener registered to multiple affected paths is called once. | ||
| */ | ||
| export function notifyChangedPaths( |
There was a problem hiding this comment.
This changes listener callback semantics even when no derived graph exists. On main, if the same callback is subscribed to root, parent, and leaf, a nested write calls it once per matching subscription. This branch dedupes it to one call total through notifyChangedPaths.
Repro on this branch prints 1; main prints 3:
const store = createBoltStore({ a: { b: 1 } });
let calls = 0;
const listener = () => calls++;
store.subscribe(undefined, listener);
store.subscribe("a", listener);
store.subscribe("a.b", listener);
store.set("a.b", 2);If that break is intentional, it needs semver/release-note treatment. Otherwise preserve the old notifyPrefixes behavior for the derivedNodes.size === 0 path and only dedupe batched derived transactions.
| * Materializes one path from other paths. Source writes settle derived targets | ||
| * before subscribers are notified. | ||
| */ | ||
| derive: BoltDerive<TState>; |
There was a problem hiding this comment.
Adding derive as a required member of BoltStoreApi is a package-facing type break for consumers who use this interface for mocks, wrappers, adapters, or tests and never use derived paths.
Existing code like this stops compiling:
const fake: BoltStoreApi<{ count: number }> = {
getState: () => ({ count: 0 }),
get: (() => 0) as BoltStoreApi<{ count: number }>["get"],
set: () => {},
subscribe: () => () => {},
};Either treat this as a semver-breaking change with migration docs, or split the public type so existing base-store mocks do not need to implement derive.
hatim-s
left a comment
There was a problem hiding this comment.
Deep review summary for PR #4:
I left inline comments for the blockers I found. The main issues are runtime ownership holes around overlapping derived paths, non-atomic writes when a derived compute throws, eager downstream recomputation after no-op upstream equality, a depth-unsafe compute reentrancy guard, and package-facing breaking changes in public types/listener semantics.
Validation I ran locally on 36dc706: bun test, bun run build:lib, bun run build, and git diff --check all pass. The comments are about edge-case correctness, compatibility, and scaling rather than current happy-path coverage.
9fef432 to
53f99e9
Compare
|
Addressed the review blockers in 53f99e9. Fixes included:
Validation:
|
Co-authored-by: GPT 5.6 Sol <codex@openai.com> Co-authored-by: Zed <zed@zed.dev>
53f99e9 to
d60167b
Compare
Co-authored-by: GPT 5.6 Sol <codex@openai.com> Co-authored-by: Zed <zed@zed.dev>
Co-authored-by: GPT 5.6 Sol <codex@openai.com> Co-authored-by: Zed <zed@zed.dev>
Summary
Validation