Repository navigation
fix: copy Set drafts of an outer create() call and warn about create() on a draft - #186
Merged
Merged
Conversation
…that its result holds
|
Coverage after merging fix/create-on-draft-warning into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Coverage after merging fix/create-on-draft-warning into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
Refs #160. Part of #168.
Summary
#160 reports that a helper calling
create()on a draft returns a value whose unchanged values are objects of the base state, so writing to them after the result is assigned back changes the base state. Immer avoids it because its nestedproducereturns drafts of the outer call, and it freezes results by default. Aligning Mutative with that (#185) changes what nested results hold everywhere, across auto-freeze, patches, strict mode, Sets,create(base)drafts and async recipes, and each review of that change found new regressions againstmain, for a pattern that one issue reports. This PR takes the other outcome that #160 lists as acceptable: it documents the behavior and warns about it in development builds. It also fixes a Set copy error found along the way.Changes, one commit per item
fix: copy Set drafts whose original is a draft of an outer create() call. Copying a Set draft whose original is a Set draft of an outer call, as increate({ set: draft.set }, …), calledSet.prototype.differenceon the outer draft, which has no Set internals, so any change or iteration threwTypeError: Method Set.prototype.difference called on incompatible receiver(mainand 1.3.0). It now copies through the draft'svalues(). The new Set test throws onmain; a Map test covers the same path for Maps, which already worked.fix: warn once in development builds that create() on a draft drafts a copy of it.create(draft, recipe)still draftscurrent(draft). Development builds now warn once per process whencreate()receives a draft with a recipe, with a link to the docs section of item 3; production builds are unchanged.docs: describe create() on a draft and the objects of the base state that its result holds. README andcreate.md: a "create()on a draft" section with the Nested create() on draft element shares references with original base object #160 helper pattern, why writing to the result's unchanged values changes the base state, the safe places to make such changes (in the helper's recipe, or through the outer draft before calling it), and the development warning.build: refresh the size baseline for the development warning of create() on a draft. The development artifacts carry the warning text; the baseline names the commit of item 2, and thesize-limitcaps are unchanged.fix: link the warning of create() on a draft to the docs instead of the issue. The code keeps no issue links: the warning points to the docs section, and the comment and test follow. The development artifacts change by a few bytes, within the size budget.Verification
test:benchmarks,size,test:package,test:build-watch,type-checkandtest(4,659 tests). Every commit passes lint, format, type checks and the suite on its own, and coverage ofsrcstays at 100%.create()calls, not committed: 9,000 random programs over synchronous recipes, async recipes before and after anawait, andcreate(base)drafts as the outer layer, with strict mode, auto-freeze and patches on either side. Helpers change their draft, return a value or callcreate()again on it, and their results are assigned back, elsewhere or discarded;create()also receives plain objects holding drafts, andcreate(base)drafts are placed in the state. The reference build ismainbefore this PR (6b08f57), not npm 1.3.0 as an earlier version of this description said: that checkout'spackage.jsonstill says 1.3.0. In a re-run on the merged code, none of the 6,674 programs that the reference runs cleanly fails or leaves a revoked draft here. Against npm 1.3.0 itself, 14 of the 6,319 programs that it runs cleanly differ here, the same 14 as onmainbefore this PR, all of themcreate()calls that receive a plain object holding drafts: 8 leave a revoked draft in the state, 5 in nested patch values, and 1 changes the base state.Size
mainsize-limitmeasures 8.3, 7.27 and 8.16 kB against the unchanged caps of 8.4, 7.4 and 8.3 kB.