-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
fix(reactivity): shallowReactive map "unwraps" the nested refs #8503
Merged
Merged
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
dfb34fd
fix(reactivity): shallowReactive map "unwraps" the nested refs
liuseen-l 6a5fb80
Merge branch 'main' of github.com:code-ManL/core-next into fix/reactive
liuseen-l 2fd7acb
feat(test): add test case for set
liuseen-l a289e54
fix(reactive): The result of assignment and value fetching is inconsi…
liuseen-l 445b7f5
Merge branch 'main' into fix/reactive
yyx990803 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a boolean flag to a function is generally something that sets off alarm bells for me. There's a lot of stuff been written on this topic, e.g. https://martinfowler.com/bliki/FlagArgument.html, but that isn't to say it's necessarily wrong.
When I first looked over this PR a few weeks ago, I felt like the boolean flag was justified here. But it kept nagging away at me, and I'm now wondering whether there's a better way.
First, I'd like to consider this example:
It's a silly example, but it shows how the flag can be passed accidentally by client code. In that example, the index is being passed by
forEach
as theisShallow
value.Does this matter? Maybe not, but it motivated me to ponder alternative implementations.
I'm now wondering whether this might be a better way to implement it:
Remove the call to
toRaw
inadd
andset
. Have them effectively be shallow by default.Override the calls to
add
andset
inmutableInstrumentations
to include thetoRaw
call. Something like:This avoids the need for the boolean flag. I tried this locally and it seemed to pass the test cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method 1 I don't think is advisable, it will lead to the contamination of the raw data, as shown in the code below, modifying the state through the raw data will cause the effect to be re-executed, and toRaw is used to avoid this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you misunderstood what I meant. I wasn't proposing two separate methods for fixing the problem, those were two parts of a single method.
I've put together an example to show the changes I had in mind:
skirtles-code@073ab1c
The calls to
toRaw
are still present, they're just moved.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, I'm sorry, I misunderstood
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding the
isShallow
arg is fine in this case because it is consistent with how other functions in the same file are structured. TheisShallow
flag is used only for value unwrapping inadd
andset
, but affects more things in other functions - in general, these handlers have to be aware of the context they are called in.