You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
feat: add the cloneDraftBase option for create() on a draft - #187
#186 documented that create() on a draft drafts current(draft): the values that a helper's recipe leaves unchanged are objects of the base state, so writing to them after the result is assigned back changes the base state. It explains the behavior and warns about it, but it leaves the reporter of #160 no way to keep their visitor helpers as they are. This PR adds an opt-in option for that: with cloneDraftBase, create() drafts a deep copy of the draft's current state, so its result shares no objects with the base state.
Without the option nothing changes, and the option costs nothing when the base state is not a draft.
Changes, one commit per item
feat: add the cloneDraftBase option to copy a draft base state before drafting it.ExternalOptions gains cloneDraftBase?: <T>(state: T) => T. When the base state is a draft, create() passes current(draft) to it and drafts the copy that it returns. It takes a function rather than a boolean: a boolean backed by the internal deepClone pulled deepClone into bundles that import only create (+108 B, over the 7.4 kB cap of the "ESM, create" size-limit entry), while the function adds 18 B. With the option set, development builds skip the warning of create() on a draft, whose text now mentions the option. Three tests, which fail without the option: the helper pattern of Nested create() on draft element shares references with original base object #160, the value that the function receives, and a makeCreator() creator used as create(draft) without a recipe on a Map and a Set.
fix: replace the drafts that current(draft) keeps when cloneDraftBase throws on them.current() returns the original of an unchanged draft, which still holds drafts when create() received an object holding drafts, as in create({ ...draft.node }, …) or create({ a: draft.a, b: draft.b }, …). structuredClone cannot copy drafts, so with item 1 alone, setting the option made such code throw DataCloneError although it works without the option. When the function throws, create() now calls it again on the current state with those drafts replaced by their current state (getCurrent(base, true)). Replacing them on every call instead would make the two cloning workloads below 15% and 31% slower, for drafts that are rarely there. The new test throws on item 1.
docs: describe the cloneDraftBase option for create() on a draft. README and create.md: an entry in the options lists, and the "create() on a draft" section lists the option among the ways to avoid the problem, with its costs (a copy of the draft on each call, new references for the values that the recipe leaves unchanged) and what structuredClone does not copy (class instances become plain objects, functions throw). The section's example now precedes those ways.
build: refresh the size baseline for the cloneDraftBase option. The baseline names the commit of item 2; the size-limit caps are unchanged.
Verification
The full Node CI sequence passes locally: lint, format, build, the benchmark checks, test:benchmarks, size, test:package, test:build-watch, type-check and test (4,663 tests). Coverage of src stays at 100%.
A regression fuzzer of nested create() calls, not committed: 9,000 random programs over synchronous recipes, async recipes before and after an await, and create(base) drafts as the outer layer, with strict mode, auto-freeze and patches on either side. Helpers change their draft, return a value or call create() again on it, and their results are assigned back, elsewhere or discarded; create() also receives plain objects holding drafts, and create(base) drafts are placed in the state.
Against main: none of the 7,084 programs that main runs cleanly fails here, without the option or with makeCreator({ cloneDraftBase: structuredClone }) for every call. With item 1 alone, 165 of them threw DataCloneError with the option, all of them calls on a draft of an object holding drafts.
main changes the base state in 60 programs, and the option fixes 52. In 6, create() receives a plain object holding drafts, so its base state is not a draft and the option does not apply; the other 2 leave an unchanged create(base) draft in the state, which is revoked when that call ends, as on main.
Against npm 1.3.0, this branch shows the same 14 differences as main, with or without the option; all of them pass create() a plain object holding drafts.
Performance, one process per measurement over 6 alternating rounds, median paired ratio to main: producers with and without patches, current() of changed objects and arrays, and nested helpers without the option measure 0.96–1.02, within the layout noise of minified builds. The option itself costs its deep copy: a helper called on each of 1,000 node drafts takes about 1.7x the time it takes without the option, and a helper on a 1,000-key object about 2.1x.
Size
Measurement
main
This PR
Δ
Production CJS artifact (Brotli)
8,114
8,169
+55
create only, bundled by a consumer (Brotli)
7,634
7,676
+42
Development CJS artifact (Brotli)
14,436
14,519
+83
Item 1 adds 18 B to the production artifact and item 2 adds 37 B. size-limit measures 8.36, 7.32 and 8.21 kB against the unchanged caps of 8.4, 7.4 and 8.3 kB, and the README bundle table stays at 7.7 and 8.2 kB.
Closing without merging in favor of #188. Measuring the pattern of #160 showed that the option is not what users should reach for: it costs a deep copy per call (about 3x Immer on that pattern once nodes hold 10 children) and gives the values that a recipe leaves unchanged new references. A helper that changes a draft in place keeps the base state unchanged, keeps those references and is the fastest approach. A helper whose result must share no objects with the base state can call create(structuredClone(current(draft)), recipe) itself, which is what the option did, and the option would only have applied to creators made by makeCreator(). #188 documents both patterns. The branch stays for reference.
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
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
#186 documented that
create()on a draft draftscurrent(draft): the values that a helper's recipe leaves unchanged are objects of the base state, so writing to them after the result is assigned back changes the base state. It explains the behavior and warns about it, but it leaves the reporter of #160 no way to keep their visitor helpers as they are. This PR adds an opt-in option for that: withcloneDraftBase,create()drafts a deep copy of the draft's current state, so its result shares no objects with the base state.Without the option nothing changes, and the option costs nothing when the base state is not a draft.
Changes, one commit per item
feat: add the cloneDraftBase option to copy a draft base state before drafting it.ExternalOptionsgainscloneDraftBase?: <T>(state: T) => T. When the base state is a draft,create()passescurrent(draft)to it and drafts the copy that it returns. It takes a function rather than a boolean: a boolean backed by the internaldeepClonepulleddeepCloneinto bundles that import onlycreate(+108 B, over the 7.4 kB cap of the "ESM, create"size-limitentry), while the function adds 18 B. With the option set, development builds skip the warning ofcreate()on a draft, whose text now mentions the option. Three tests, which fail without the option: the helper pattern of Nested create() on draft element shares references with original base object #160, the value that the function receives, and amakeCreator()creator used ascreate(draft)without a recipe on a Map and a Set.fix: replace the drafts that current(draft) keeps when cloneDraftBase throws on them.current()returns the original of an unchanged draft, which still holds drafts whencreate()received an object holding drafts, as increate({ ...draft.node }, …)orcreate({ a: draft.a, b: draft.b }, …).structuredClonecannot copy drafts, so with item 1 alone, setting the option made such code throwDataCloneErroralthough it works without the option. When the function throws,create()now calls it again on the current state with those drafts replaced by their current state (getCurrent(base, true)). Replacing them on every call instead would make the two cloning workloads below 15% and 31% slower, for drafts that are rarely there. The new test throws on item 1.docs: describe the cloneDraftBase option for create() on a draft. README andcreate.md: an entry in the options lists, and the "create() on a draft" section lists the option among the ways to avoid the problem, with its costs (a copy of the draft on each call, new references for the values that the recipe leaves unchanged) and whatstructuredClonedoes not copy (class instances become plain objects, functions throw). The section's example now precedes those ways.build: refresh the size baseline for the cloneDraftBase option. The baseline names the commit of item 2; thesize-limitcaps are unchanged.Verification
test:benchmarks,size,test:package,test:build-watch,type-checkandtest(4,663 tests). 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.main: none of the 7,084 programs thatmainruns cleanly fails here, without the option or withmakeCreator({ cloneDraftBase: structuredClone })for every call. With item 1 alone, 165 of them threwDataCloneErrorwith the option, all of them calls on a draft of an object holding drafts.mainchanges the base state in 60 programs, and the option fixes 52. In 6,create()receives a plain object holding drafts, so its base state is not a draft and the option does not apply; the other 2 leave an unchangedcreate(base)draft in the state, which is revoked when that call ends, as onmain.main, with or without the option; all of them passcreate()a plain object holding drafts.main: producers with and without patches,current()of changed objects and arrays, and nested helpers without the option measure 0.96–1.02, within the layout noise of minified builds. The option itself costs its deep copy: a helper called on each of 1,000 node drafts takes about 1.7x the time it takes without the option, and a helper on a 1,000-key object about 2.1x.Size
maincreateonly, bundled by a consumer (Brotli)Item 1 adds 18 B to the production artifact and item 2 adds 37 B.
size-limitmeasures 8.36, 7.32 and 8.21 kB against the unchanged caps of 8.4, 7.4 and 8.3 kB, and the README bundle table stays at 7.7 and 8.2 kB.