components: Stop modifying the parent context and correctly memoize it #32745
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
sarayourfriend
added
[Type] Bug
sarayourfriend
requested review from
ajitbohra and
chrisvanpatten
as code owners
|
Size Change: -14 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
ciampo
reviewed
Jun 16, 2021
There was a problem hiding this comment.
Thank you for looking into this! It definitely feels like a step in the right direction, but after a quick check it doesn't seem to fix all of the issues that I flagged in #32743
Fixed:
- The context "leak" problem
Still not fixed:
- The inner most
Carddoesn't seem to inherit values from the outer context (e.g. the innermost card doesn't have anelevationof 10 - When swapping from
ui/cardtosrc/card, the values defined in the story's context still don't seem to apply to theCardcomponents
@sarayourfriend can you reproduce this same behaviour?
I can reproduce this problem. I'll see if I can fiddle with it tomorrow to discover what's going on. This might just be the actual expected behavior of the context system, I'm not sure. |
sarayourfriend
changed the title
sarayourfriend
commented
Jun 17, 2021
|
|
||
| if ( ! isEqual( value, valueRef.current ) ) { | ||
| valueRef.current = value; |
There was a problem hiding this comment.
It turns out this was the problematic line that was causing the context to not get merged correctly. Becuase valueRef was being assigned to the merged value, value and valueRef.current were never going to be equal if the parent context included anything.
We're able to avoid this comparison altogether if we make a guarantee that value and parent context are memoized however, so the new version of this hook is able to avoid this complexity completely.
ciampo
approved these changes
Jun 18, 2021
| }, [ value, parentContext ] ); | ||
| // parent context will always be memoized or the default value (which will not change) | ||
| // so this memoization will prevent `merge` and `cloneDeep` from rerunning unless | ||
| // the referneces to value change. The `useUpdateEffect` above will ensure that we are |
There was a problem hiding this comment.
Suggested change
| // the referneces to value change. The `useUpdateEffect` above will ensure that we are | |
| // the references to value change. The `useUpdateEffect` above will ensure that we are |
| setConfig( ( prev ) => ( { ...prev, ...valueRef.current } ) ); | ||
| } | ||
| }, [ value, parentContext ] ); | ||
| // parent context will always be memoized or the default value (which will not change) |
There was a problem hiding this comment.
I'm not sure I follow completely the meaning of the first 2 sentences, would you mind articulating a bit better?
There was a problem hiding this comment.
I tried to re-write this/add more details, but I'm not sure it actually is helpful. Let me know what you think.
There was a problem hiding this comment.
Much better!
It's important that this function's implementation is well commented, since it's very dense and empowers the whole context system. Thank you!
sarayourfriend
force-pushed
the
fix/do-not-modify-parent-context
branch
from
d1910b8
to
db4da2c
Compare
This was referenced Jun 22, 2021
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.
Description
This fixes a bug in the
ContextSystemProviderwhere the parent context was being modified by themergefunction which is not a pure function (hello lodash 😱). By cloning the parent context we ensure that it will not be modified and therefore spread throughout the app.To test this apply this patch to #32743 and ensure that you can no longer reproduce the bug described by @ciampo.
Note: if this becomes a performance concern we can always switch to using the
deepmergepackage which exports a pure function. It's very fast and very small dependency. May be worth it if we find this to be a perf issue.How has this been tested?
Apply the patch to the PR described above.
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).