Repository navigation
fix: reject Map keys that are not strings and symbol keys in string patch paths in development builds - #189
Merged
Merged
Conversation
|
Coverage after merging fix/string-path-keys into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Coverage after merging fix/string-path-keys into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
unadlib
changed the title
This was referenced Oct 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #168.
Summary
With
enablePatches: { pathAsArray: false }, patch paths are JSON Pointer strings, andescapePath()turns every key into a string. An external review that led to #184 noted under its finding B2 that a string path cannot hold a Map key with object identity; #184 fixed the array-path case only. The string-path case is broader, onmainand on npm 1.3.0 alike:pathAsArray: falseapply(base, patches)draft.set(1, 'b')onMap { 1 => 'a' }/1Map { 1 => 'a', '1' => 'b' }draft.set(true, 'b')/true'true'draft.set(key, 'b')withkey = ['k']/k'k'draft.set(key, 'b')withkey = { id: 1 }/[object Object]'[object Object]'draft.get(1).v = 2/1/vCannot apply patch at '1/v'create()throwsTypeError: Cannot convert a Symbol value to a stringThe first four write another key without any error. A JSON Pointer holds only strings, so these keys cannot be encoded. With this PR, development builds throw a clear error for them, and the production artifacts stay byte-identical to
main.Why development builds only
The first version of this PR threw in production builds too, with a new error code 21. Reviewing it for correctness and necessity found:
mark()copy function together with patches or auto-freeze treats a similar misuse the same way.getPath()checked each key while walking up, before it knew whether the path resolves. When a changed Map left its key afterwards (moved, deleted, replaced, or reordered byreverse()), the first version threw althoughmainand 1.3.0 generate correct patches there: the stale path is dropped, and the parent's patches carry the Map's value.Production builds keep the v1 behavior, which the docs describe.
Changes, one commit per item
fix: reject Map keys that are not strings and symbol keys in string patch paths:checkPathKey()runs where keys enter a path, ingetPath()for the parents of a changed draft (Sets keep their positions) and ingeneratePatchesFromAssigned()for assigned keys. The message names the key's type rather than its value, becauseString(key)prints['k']askand throws for objects without a prototype. Addstest/string-path-keys.test.ts, error code 21 and a migration guide bullet; item 5 removes the code and moves the bullet.docs: note that string patch paths support only string Map keys and no symbol keys.build: refresh the size baseline for the string patch path key check.fix: check string patch path keys only once the path resolves:getPath()checks a key after the recursive call returns a path, so a path that a level above drops is never checked. Adds a test of five such recipes, which fails on item 1.fix: check string patch path keys only in development builds: both call sites run under__DEV__and the message is inline, as for the other development-only checks, so error code 21, its row on the errors page and its production test are gone. The type comes fromoriginal, because Terser kept an unused destructuredtype(7 B). The migration guide bullet moves from "Patches" to "Development builds", and a production test pins that production builds skip the check.docs: say that development builds reject keys a string patch path cannot name: the option docs in the README,create.mdand the patches guide. The previous wording also did not fit symbol keys, which never produced patches.build: refresh the size baseline for the development check of string patch path keys.String Map keys, including keys with
/and~, default array paths and producers without patches are unchanged.Behavior
pathAsArray: falsemainand 1.3.0mainmainTypeErrorTypeError, asmainreverse()Known limitations
Development builds can throw for a recipe whose patches hold no such key:
push()thenpop()on an array, setting an entry of a Map back, or reversing an array twice. The path is built before the patches show that nothing changed; avoiding this needs a check at patch emission, which is not worth the code for a recipe that changes nothing.draft.map.get(1).v = 2; return draft.other;. The returned value replaces the patches with onereplaceat the root, but the patches of the changed drafts are still generated, and their paths are checked. Skipping patch generation when a recipe returns a value would remove this case for 26 B raw in the production CJS artifact.With the two-step API,
const [draft, finalize] = create(base, { enablePatches: { pathAsArray: false } }), afinalize()that throws this error leaves the drafts alive, and callingfinalize()again returns a state that holds a revoked draft and patches without the failed part. Recipe producers revoke their drafts on any error since #184; the two-step API does not, and this check adds an error that can be thrown while it finalizes. Revoking there would cost 46 B raw in the production CJS artifact.These cases need recipes that are rare even among those that use string paths with such keys, and they only affect development builds, so none of them is worth production bytes. A differential fuzz of 50,000 random recipes against the production build found no patch path holding such a key that development builds let through; every development error without one came from the first two cases.
Verification
test:benchmarks,size,test:package,test:build-watch,type-checkandtest(4,668 tests). Coverage ofsrcis 100% of lines, branches and functions.test/string-path-keys.test.tsfail onmain: the threerejecttests and the revocation test. The test of containers that left their key fails on item 1.main.Size
The production artifacts (CJS, UMD and ESM) are byte-identical to
main, so production runs the same code as before.mainesm.js/esm.mjs), rawConsumer bundles built with
NODE_ENV=productionkeep their raw sizes; their Brotli sizes move by −4 to +11 B.size-limitmeasures 8.3, 7.27 and 8.16 kB, as onmain, against the caps of 8.4, 7.4 and 8.3 kB.