-
-
Notifications
You must be signed in to change notification settings - Fork 851
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Optional Support For Multiple References to an Object #1088
Open
BellCubeDev
wants to merge
8
commits into
immerjs:main
Choose a base branch
from
BellCubeDev:main
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains 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
BellCubeDev
commented
Jan 10, 2024
BellCubeDev
force-pushed
the
main
branch
3 times, most recently
from
February 4, 2024 02:53
eebb9e2
to
83e87ff
Compare
# What This Does Some state trees may need to reference an object more than once (such as the tree for my [fomod](https://www.npmjs.com/package/fomod) library). In essence, we store existing drafts when an off-by-default Immer class configuration option is enabled. This should be a painless solution. Specifics are described below. ## Implementation Details * Two `WeakMap` are used to keep track of draft states and related data at different parts of the immerification process: * `existingStateMap_` maps a given base object to the first draft state created for it. This state includes a reference to the revokable draft. * If a state is referenced multiple times, it will be given a new `revoke_()` function that, once called the first time, calls the old `revoke_()` function. The result is that the final `revoke_()` must be called once for every requested draft before the Proxy is finally revoked. Since a proxy which has has its `revoke_()` method called should be considered revoked by all code paths, duplicate calls should not be an issue. * During finalization, `encounteredObjects` keeps track of objects we've finalized and doesn't traverse an object if it's already seen it. It prevents infinite recursion when circular references are present. * Introduced the `extraParents_` property to the `ImmerBaseState` interface. This keeps track of additional values that would normally be attached to `parent_` so that functionality such as marking the parent state as modified is retained for objects with multiple parent objects * For Maps and Sets, a proxy is established between the state and DraftMap/DraftSet classes to handle multiple references to these native classes while preserving the idea of having one DraftSet per reference. * For Sets, each child draft has a single symbol value set so that a copy is prepared. (discussion needed; see TODOs below) * During finalization, objects may have drafted children and, thus, even unmodified children are finalized in multi-ref mode * To enable the feature, it is the same as other Immer class configuration options (such as `useStrictShallowCopy`). That is, either specify it in the config object passed to the class's constructor OR call the relevant method, `setAllowMultiRefs()` > [!NOTE] > Because of the extra computation involved with checking every proxied object against a map and traversing every object in a tree, enabling multi-ref will have a significant performance impact—even on trees which contain no repeated references. # Tests The file `__tests__/multiref.ts` contains a number of tests related to this multi-reference support implementation. Such tests seek to verify that: * Direct circular references (which Immer tests for normally) do not throw an error when multi-ref is enabled * When the properties of multiple references are modified, all references are modified * Unmodified references to the same object are kept * The same copy is provided for every reference (new references are strictly equivalent [`===`] just as the references before `produce()` would have been) Tests are performed on all relevant object archetypes where applicable. # Outstanding Discussion TODOs * [ ] What to do regarding documentation * [ ] Possible alternate solution for preparing copies for multi-reference DraftSet children * [ ] Add an error for when WeakMap isn't supported in the current environment? (supported in every noteworthy browser and server environment since late 2015)
Cuts down on value passing and works better with nested producers.
See new tests and comments in multiref.ts for elaboration regarding the extended multiref support included in this commit.
👀 left those in by mistake
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What This Does
Some state trees may need to reference an object more than once (such as the tree for my fomod library). In essence, we store existing drafts when an off-by-default Immer class configuration option is enabled. This should be a painless solution. Specifics are described below.
Implementation Details
WeakMap
are used to keep track of draft states and related data at different parts of the immerification process:existingStateMap_
maps a given base object to the first draft state created for it. This state includes a reference to the revokable draft.revoke_()
function that, once called the first time, calls the oldrevoke_()
function. The result is that the finalrevoke_()
must be called once for every requested draft before the Proxy is finally revoked. Since a proxy which has has itsrevoke_()
method called should be considered revoked by all code paths, duplicate calls should not be an issue.encounteredObjects
keeps track of objects we've finalized and doesn't traverse an object if it's already seen it. It prevents infinite recursion when circular references are present.extraParents_
property to theImmerBaseState
interface. This property keeps track of additional values that would normally be attached toparent_
so that functionality relying on parent_ (such as marking the parent state as modified) is retained for objects with multiple parent objectsuseStrictShallowCopy
). That is, either specify it in the config object passed to the class's constructor OR call the relevant class method,setAllowMultiRefs()
Note
Because of the extra computation involved with checking every proxied object against a map and traversing every object in a tree, enabling multi-ref will have a significant performance impact—even on trees which contain no repeated references.
Tests
The file
__tests__/multiref.ts
contains a number of tests related to this multi-reference support implementation. Such tests seek to verify that:===
] just as the references beforeproduce()
would have been)Additionally, I've added
allowMultiRefs
into the base.js test matrix and adjusted tests as necessary to accommodate forallowMultiRefs
' required functionality.Tests are performed on all relevant object archetypes where applicable.
Outstanding Discussion TODOs
Note
I have yet to compare the performance of stock Immer to the version of Immer created by my pull request without
allowMultiRefs
enabled. There may be no significant difference but work might be needed to address significant performance regressions should they exist.Caution
Some tests are still failing when
allowMultiRefs
is set totrue
. That said, NO TESTS FAIL WHENallowMultiRefs
IS SET TOfalse
.