Fix serial passive effects #15650
Merged
Fix serial passive effects #15650
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
Currently, we check for pending passive effects inside the `setState` method before we add additional updates to the queue, in case those pending effects also add things to the queue. However, the `setState` method is too late, because the event that caused the update might not have ever fired had the passive effects flushed before we got there. This is the same as the discrete/serial events problem. When a serial update comes in, and there's already a pending serial update, we have to do it before we call the user-provided event handlers. Because the event handlers themselves might change as a result of the pending update. This commit moves the `flushPassiveEffects` call to before the discrete event handlers are called, and removes it from the `setState` method. Non-discrete events will not cause passive effects to flush, which is fine, since by definition they are not order dependent.
|
@threepointone This is a duplicate of #15644 that I started last week but never opened a PR for. I don't care which one we merge as long as you incorporate the semantic changes and tests in this one. |
|
I think the only difference between our PRs is that I removed the |
|
ReactDOM: size: -0.1%, gzip: -0.2% Details of bundled changes.Comparing: af19e2e...cb56090 react-dom
react-art
react-native-renderer
Generated by 🚫 dangerJS |
|
If you need to treat setState as serial (eg from keyboard window event) how do you opt in? Are making interactiveUpdates a public API? Do we rename it? |
|
@gaearon In a real test I would simulate a click event. Since ReactNoop doesn't have an event system, I used |
threepointone
approved these changes
May 15, 2019
|
I guess I’m asking what’s the upgrade path for people who wanted to rely on serial behavior and can’t anymore with this change. Shouldn’t that upgrade path be a part of the same release? |
|
We already lack a first class way to create custom serial event handlers (e.g. key events in #14750). You can get most of the way there with event delegation: attach a single native event listener and wrap all the handlers in You'd still need a way to force flush passive effects. We could expose Regardless, that problem exists independently of the problem fixed by this PR (where |
acdlite
added a commit
to acdlite/react
that referenced
this pull request
May 15, 2019
PR facebook#15650 is a bugfix but it's technically a semantic change that could cause regressions. I don't think it will be an issue, since the previous behavior was both broken and incoherent, but out of an abundance of caution, let's wrap it in a flag so we can easily revert it if necessary.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
May 15, 2019
PR facebook#15650 is a bugfix but it's technically a semantic change that could cause regressions. I don't think it will be an issue, since the previous behavior was both broken and incoherent, but out of an abundance of caution, let's wrap it in a flag so we can easily revert it if necessary.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
May 15, 2019
PR facebook#15650 is a bugfix but it's technically a semantic change that could cause regressions. I don't think it will be an issue, since the previous behavior was both broken and incoherent, but out of an abundance of caution, let's wrap it in a flag so we can easily revert it if necessary.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
May 15, 2019
PR facebook#15650 is a bugfix but it's technically a semantic change that could cause regressions. I don't think it will be an issue, since the previous behavior was both broken and incoherent, but out of an abundance of caution, let's wrap it in a flag so we can easily revert it if necessary.
acdlite
added a commit
that referenced
this pull request
May 15, 2019
PR #15650 is a bugfix but it's technically a semantic change that could cause regressions. I don't think it will be an issue, since the previous behavior was both broken and incoherent, but out of an abundance of caution, let's wrap it in a flag so we can easily revert it if necessary.
This was referenced Jul 30, 2019
Merged
Merged
5 tasks
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Aug 12, 2019
The change in facebook#15650 has fully rolled out, so we can remove the flag that reverts it.
acdlite
added a commit
that referenced
this pull request
Aug 12, 2019
This was referenced Mar 10, 2020
This was referenced Mar 18, 2020
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.
Flush passive effects before discrete events. Currently, we check for pending passive effects inside the
setStatemethod before we add additional updates to the queue, in case those pending effects also add things to the queue.However, the
setStatemethod is too late, because the event that caused the update might not have ever fired had the passive effects flushed before we got there.This is the same as the discrete/serial events problem. When a serial update comes in, and there's already a pending serial update, we have to do it before we call the user-provided event handlers. Because the event handlers themselves might change as a result of the pending update.
This commit moves the
flushPassiveEffectscall to before the discrete event handlers are called, and removes it from thesetStatemethod. Non-discrete events will not cause passive effects to flush, which is fine, since by definition they are not order dependent.