Capture state changes scheduled between render and effect#38509
Merged
dcalhoun merged 7 commits intoSep 16, 2022
Conversation
dcalhoun
added
Mobile App - i.e. Android or iOS
|
Size Change: +2.47 kB (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
dcalhoun
force-pushed
the
bug/capture-state-changes-before-use-select-subscription
branch
3 times, most recently
from
8b4c4d7 to
3987f61
Compare
7 tasks
dcalhoun
force-pushed
the
bug/capture-state-changes-before-use-select-subscription
branch
from
9fb39e1 to
bd255ac
Compare
In the event that a child component dispatched an action modifying the store, the changes in the store were lost due to `useSelect`'s `useIsomorphicLayoutEffect` referencing a stale value.
In the event that a child component dispatched an action modifying the store, the changes in the store were lost due to `useSelect`'s `useIsomorphicLayoutEffect` referencing a stale value. This change ensures the `useIsomorphicLayoutEffect` references the latest `mapOutput` when setting the related `latestMapOutput.current` value.
Store updates triggered from within the post title component's `componentDidUpdate` hook resulted in stale `isTitledSelected` values returned from the memoized `useSelect`. This reverts a workaround that avoided memoization by splitting the `useSelect` hook usage in two.
dcalhoun
force-pushed
the
bug/capture-state-changes-before-use-select-subscription
branch
from
f804f04 to
3172233
Compare
Member
Author
|
Hey @nerrad and @youknowriad. 👋🏻 This change touches fairly low-level logic. From what I found, you both have discussed the existing code a good bit. I would appreciate your perspective on these changes. It is not a rush, so please review at your convenience. Thank you! 🙇🏻 |
Member
|
Hi @dcalhoun 👋 seems you're really up to something 🙂 The // read the current value
const mapOutput = latestMapOutput.current;
// indiscriminately write the read value back into the ref in an effect
useIsomorphicLayoutEffect( () => {
latestMapOutput.current = mapOutput;
} );
// write to the ref also in subscription callback
subscribe( function onStoreChange() {
latestMapOutput.current = newValue;
} );Your test example works in such a way that the
At the moment when the action is being dispatched the |
Member
|
I remembered that the I rewrote the unit test that this PR is adding to Redux and observed that a stale value is really written:
So, the stale value is written, but it will never be used. On the other hand, |
jsnajdr
reviewed
Sep 15, 2022
This avoids stale mappings overwriting new values in specific contexts where `onStoreChange` updates the value before the effect writes the original mapping.
Member
Author
|
Thank you for the thorough review and investigation, @jsnajdr. 🙇🏻
This makes sense now that you explain it so clearly.
This is very interesting, and very educational for me in regards to how |
dcalhoun
changed the title
dcalhoun
commented
Sep 15, 2022
jsnajdr
approved these changes
Sep 16, 2022
This test focuses upon the order in which state is read and written. It is not related to when the subscription is set.
Reduce cognitive load of code by grouping related logic.
dcalhoun
changed the title
dcalhoun
deleted the
bug/capture-state-changes-before-use-select-subscription
branch
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
2 participants
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?
Fixes #32154. Supersedes #32831. Avoid stale mappings overwriting new state in specific contexts where
onStoreChangeupdates the state before the effect writes the initial mapping.Why?
In the event that a child component dispatched an action modifying the store before the selector mapping was written, the changes in the store were lost due to
useSelect'suseIsomorphicLayoutEffectreferencing a stale value.A specific example of this issue occurring is the following code where
onUnselectis called. This call triggers an update to the store the occurs in betweenuseSelect's reading and writing the selector value, resulting in a staleisTitleSelectedvalue. This resulted in the blocks inserted to the incorrect location, which is captured in #32154.gutenberg/packages/editor/src/components/post-title/index.native.js
Lines 36 to 48 in 3987f61
gutenberg/packages/editor/src/components/provider/use-block-editor-settings.native.js
Line 31 in 3987f61
How?
Avoid setting a reference to the selector output mapping until after the selector is run.
Testing Instructions
The following was manually tested:
Screenshots or screencast
Before
before.mov
After
after.mov