From 471fcd8e3d55d4c0113bf4b0e6fbb060ba1a3b0d Mon Sep 17 00:00:00 2001 From: Bob Massarczyk Date: Thu, 13 Aug 2026 15:59:17 +0200 Subject: [PATCH] fix(config): say what is wrong when a path is keyed and replaced Configuring keyBy and replace on the same path reported that the replace path "makes the keyBy path unreachable". Nothing is unreachable: the two policies sit on the same node, one saying to enter the list and the other saying it is opaque. They contradict rather than nest, and the old wording sent the reader looking for a nesting problem that is not there. It also withheld the spelling that works, which is not guessable. The message now states the contradiction and names the item-swap spelling. It deliberately stops there rather than prescribing an edit. A first attempt did prescribe one, and both reviewers found configurations where the prescription throws again: a keyBy nested below the list strands under either replace spelling, and a second colliding replace path defeats the advice even with no nested policy. Guaranteeing that a suggested edit compiles would mean solving the whole policy graph inside one error, so the message reports the collision it found and names a spelling, both of which stay true whatever else is configured. Genuinely nested collisions keep the unreachable wording, which is accurate for them. The README documented neither case at the same path, so it does now. Co-Authored-By: Claude Opus 5 --- README.md | 4 ++-- src/paths.ts | 17 +++++++++++++++-- test/config.test.ts | 25 ++++++++++++++++++++++++- 3 files changed, 41 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index ed14a94..38cb6fa 100644 --- a/README.md +++ b/README.md @@ -94,7 +94,7 @@ const merge = createMerger({ `keyBy` maps a list path to its identity field. Identity values must be stable, unique strings or numbers. Matching is strict, so `1` and `"1"` are different items, while `0` and `""` are valid identities. Missing, duplicate, `NaN`, and non-string/non-number identities throw. -`replace` makes a path swap wholesale instead of deep-merging or reconciling. A wholesale swap never recurses, so `createMerger` rejects any policy nested below a replaced path. The `"order.items[]"` form is the item-swap idiom: the list still matches items by identity, but each matched item is replaced by its incoming value instead of merged: +`replace` makes a path swap wholesale instead of deep-merging or reconciling. A wholesale swap never recurses, so `createMerger` rejects any policy nested below a replaced path. Keying and replacing the same path is rejected too: one says to enter the list and the other says it is opaque, so they contradict rather than nest. The `"order.items[]"` form is the item-swap idiom: the list still matches items by identity, but each matched item is replaced by its incoming value instead of merged: ```ts const replaceItems = createMerger({ @@ -182,7 +182,7 @@ Paths are dot-separated property names. `[]` means “inside each keyed item of There are no wildcards, indices, root tokens, or escaping. Properties containing `.`, `[`, or `]` are not addressable in v1. The root cannot be keyed; wrap a top-level array in an object when it needs reconciliation. -All configuration is validated when the merger is created: bad grammar, reserved names, duplicates, `[]` segments under lists that have no key, and policies made unreachable by a broader `replace` all throw. Paths are never checked against `T` or runtime data. +All configuration is validated when the merger is created: bad grammar, reserved names, duplicates, `[]` segments under lists that have no key, policies made unreachable by a broader `replace`, and a path that is both keyed and replaced all throw. Paths are never checked against `T` or runtime data. ## Semantics diff --git a/src/paths.ts b/src/paths.ts index d4b614f..1368818 100644 --- a/src/paths.ts +++ b/src/paths.ts @@ -87,11 +87,24 @@ export function compileOptions(options: MergeOptions): CompiledOptions { // itself; the reverse nesting ('[]' under a keyBy list) is the // load-bearing item-swap idiom and passes. for (const keyed of keyPolicies) { - if (isPrefix(policy.segments, keyed.segments)) { + if (!isPrefix(policy.segments, keyed.segments)) continue; + // Equal paths do not nest. Neither policy shadows the other: one says + // the list is opaque and the other says to enter it, so the config + // contradicts itself rather than stranding a subtree. + // A keyBy path never ends in '[]', so appending it always names the + // item-swap spelling, which is not guessable from the error. Name the + // spelling and stop there: what else the caller has configured decides + // whether the rest compiles, and this message cannot know that. + if (policy.segments.length === keyed.segments.length) { throw new KeyfoldConfigError( - `replace path '${policy.path}' makes keyBy path '${keyed.path}' unreachable`, + `path '${policy.path}' cannot be both keyed and replaced: keyBy enters the list, ` + + `replace treats it as opaque; replacing matched items instead is spelled ` + + `'${policy.path}[]'`, ); } + throw new KeyfoldConfigError( + `replace path '${policy.path}' makes keyBy path '${keyed.path}' unreachable`, + ); } for (const other of replacePolicies) { if ( diff --git a/test/config.test.ts b/test/config.test.ts index c50ada9..dadc14c 100644 --- a/test/config.test.ts +++ b/test/config.test.ts @@ -46,7 +46,6 @@ describe("configuration validation", () => { }); const unreachableOptions: MergeOptions[] = [ - { keyBy: { "order.items": "id" }, replace: ["order.items"] }, { keyBy: { "order.items": "id" }, replace: ["order"] }, { keyBy: { "order.items": "id", "order.items[].components": "sku" }, @@ -61,6 +60,30 @@ describe("configuration validation", () => { }, ); + test("names the item-swap idiom when one path is both keyed and replaced", () => { + // Nothing is stranded here, so 'unreachable' would misdescribe it: the two + // policies contradict each other at the same node. The message has to name + // the item-swap spelling, because that spelling is not guessable. + const collide = () => + createMerger({ keyBy: { "order.items": "id" }, replace: ["order.items"] }); + + expect(collide).toThrow(KeyfoldConfigError); + expect(collide).toThrow(/cannot be both keyed and replaced/); + expect(collide).toThrow(/'order\.items\[\]'/); + expect(collide).not.toThrow(/unreachable/); + }); + + test("names the item-swap spelling without prescribing the rest of the config", () => { + // The message reports one collision; it cannot know whether the caller's + // other policies also collide. Naming a spelling stays true either way, + // where telling them what to do would not. + const alsoCollidesElsewhere = () => + createMerger({ keyBy: { items: "id" }, replace: ["items", "items[].parts"] }); + + expect(alsoCollidesElsewhere).toThrow(/'items\[\]'/); + expect(alsoCollidesElsewhere).not.toThrow(/\b(drop|remove|use replace path)\b/); + }); + test("rejects a replace path shadowed by a broader replace path", () => { expect(() => createMerger({ replace: ["a", "a.b"] })).toThrow(/unreachable/); expect(() => createMerger({ replace: ["a.b", "a"] })).toThrow(/unreachable/);