Skip to content
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 serial passive effects #15650

Merged
merged 2 commits into from May 15, 2019
Merged

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented May 14, 2019

Flush passive effects before discrete events. 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.

gaearon and others added 2 commits May 14, 2019 10:47
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.
@acdlite
Copy link
Collaborator Author

acdlite commented May 14, 2019

@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.

@acdlite
Copy link
Collaborator Author

acdlite commented May 14, 2019

I think the only difference between our PRs is that I removed the flushPassiveEffects call from setState, for the reason described above.

@sizebot
Copy link

sizebot commented May 14, 2019

ReactDOM: size: -0.1%, gzip: -0.2%

Details of bundled changes.

Comparing: af19e2e...cb56090

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js 0.0% 0.0% 829.22 KB 829.24 KB 189.33 KB 189.34 KB UMD_DEV
react-dom.production.min.js -0.1% -0.2% 104.11 KB 104.02 KB 33.81 KB 33.76 KB UMD_PROD
react-dom.profiling.min.js -0.1% -0.2% 107.26 KB 107.18 KB 34.79 KB 34.73 KB UMD_PROFILING
react-dom.development.js 0.0% 0.0% 823.63 KB 823.64 KB 187.77 KB 187.78 KB NODE_DEV
react-dom.production.min.js -0.1% -0.1% 104.1 KB 104.02 KB 33.25 KB 33.21 KB NODE_PROD
react-dom.profiling.min.js -0.1% -0.1% 107.45 KB 107.36 KB 34.17 KB 34.12 KB NODE_PROFILING
ReactDOM-dev.js 0.0% 0.0% 848.22 KB 848.23 KB 189.3 KB 189.32 KB FB_WWW_DEV
ReactDOM-prod.js -0.2% -0.2% 348.25 KB 347.6 KB 64.51 KB 64.36 KB FB_WWW_PROD
ReactDOM-profiling.js -0.2% -0.2% 353.45 KB 352.73 KB 65.51 KB 65.35 KB FB_WWW_PROFILING
react-dom-unstable-fire.development.js 0.0% 0.0% 829.57 KB 829.58 KB 189.48 KB 189.49 KB UMD_DEV
react-dom-unstable-fire.production.min.js -0.1% -0.2% 104.12 KB 104.04 KB 33.82 KB 33.77 KB UMD_PROD
react-dom-unstable-fire.profiling.min.js -0.1% -0.2% 107.28 KB 107.19 KB 34.79 KB 34.74 KB UMD_PROFILING
react-dom-unstable-fire.development.js 0.0% 0.0% 823.97 KB 823.99 KB 187.92 KB 187.93 KB NODE_DEV
react-dom-unstable-fire.production.min.js -0.1% -0.1% 104.12 KB 104.03 KB 33.26 KB 33.22 KB NODE_PROD
react-dom-unstable-fire.profiling.min.js -0.1% -0.1% 107.46 KB 107.38 KB 34.18 KB 34.13 KB NODE_PROFILING
ReactFire-dev.js 0.0% 0.0% 847.43 KB 847.44 KB 189.28 KB 189.3 KB FB_WWW_DEV
ReactFire-prod.js -0.2% -0.2% 336.22 KB 335.58 KB 62.04 KB 61.9 KB FB_WWW_PROD
ReactFire-profiling.js -0.2% -0.2% 341.4 KB 340.68 KB 63 KB 62.85 KB FB_WWW_PROFILING
react-dom-test-utils.development.js 0.0% 0.0% 54.38 KB 54.38 KB 15.04 KB 15.04 KB UMD_DEV
react-dom-test-utils.production.min.js 0.0% 0.0% 10.56 KB 10.56 KB 3.89 KB 3.9 KB UMD_PROD
react-dom-test-utils.development.js 0.0% 0.0% 54.1 KB 54.1 KB 14.97 KB 14.97 KB NODE_DEV
react-dom-test-utils.production.min.js 0.0% 0.0% 10.35 KB 10.35 KB 3.82 KB 3.82 KB NODE_PROD
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 60.76 KB 60.76 KB 15.84 KB 15.85 KB UMD_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% 🔺+0.1% 10.69 KB 10.69 KB 3.67 KB 3.67 KB UMD_PROD
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 60.43 KB 60.43 KB 15.72 KB 15.72 KB NODE_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% 0.0% 10.43 KB 10.43 KB 3.56 KB 3.57 KB NODE_PROD
react-dom-server.browser.production.min.js 0.0% 0.0% 19.11 KB 19.11 KB 7.21 KB 7.21 KB UMD_PROD
react-dom-server.browser.development.js 0.0% 0.0% 132.94 KB 132.94 KB 35.1 KB 35.1 KB NODE_DEV
react-dom-server.browser.production.min.js 0.0% 0.0% 19.03 KB 19.03 KB 7.2 KB 7.2 KB NODE_PROD
ReactDOMServer-dev.js 0.0% 0.0% 135.29 KB 135.29 KB 34.76 KB 34.76 KB FB_WWW_DEV
ReactDOMServer-prod.js 0.0% 0.0% 47.78 KB 47.78 KB 10.98 KB 10.98 KB FB_WWW_PROD
react-dom-server.node.development.js 0.0% 0.0% 134.89 KB 134.89 KB 35.65 KB 35.65 KB NODE_DEV
react-dom-server.node.production.min.js 0.0% 0.0% 19.9 KB 19.9 KB 7.51 KB 7.51 KB NODE_PROD
react-dom-unstable-fizz.browser.development.js 0.0% +0.1% 3.81 KB 3.81 KB 1.53 KB 1.54 KB UMD_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.1% 1.21 KB 1.21 KB 705 B 706 B UMD_PROD
react-dom-unstable-fizz.browser.development.js 0.0% +0.1% 3.64 KB 3.64 KB 1.49 KB 1.49 KB NODE_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.2% 1.05 KB 1.05 KB 636 B 637 B NODE_PROD
react-dom-unstable-fizz.node.development.js 0.0% +0.1% 3.88 KB 3.88 KB 1.51 KB 1.51 KB NODE_DEV
react-dom-unstable-fizz.node.production.min.js 0.0% 🔺+0.2% 1.1 KB 1.1 KB 666 B 667 B NODE_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js -0.0% -0.0% 565.04 KB 564.83 KB 123.96 KB 123.91 KB UMD_DEV
react-art.production.min.js -0.1% -0.1% 95.93 KB 95.83 KB 29.46 KB 29.43 KB UMD_PROD
react-art.development.js -0.0% -0.1% 496 KB 495.79 KB 106.54 KB 106.49 KB NODE_DEV
react-art.production.min.js -0.2% -0.3% 60.91 KB 60.81 KB 18.75 KB 18.69 KB NODE_PROD
ReactART-dev.js -0.0% -0.0% 505.46 KB 505.25 KB 105.68 KB 105.64 KB FB_WWW_DEV
ReactART-prod.js -0.4% -0.5% 196.88 KB 196.11 KB 33.46 KB 33.3 KB FB_WWW_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js 0.0% 0.0% 637.47 KB 637.49 KB 135.96 KB 135.97 KB RN_FB_DEV
ReactNativeRenderer-prod.js -0.3% -0.4% 245.19 KB 244.38 KB 42.57 KB 42.4 KB RN_FB_PROD
ReactNativeRenderer-profiling.js -0.3% -0.4% 253.15 KB 252.41 KB 44.22 KB 44.06 KB RN_FB_PROFILING
ReactNativeRenderer-dev.js 0.0% 0.0% 637.39 KB 637.4 KB 135.93 KB 135.94 KB RN_OSS_DEV
ReactNativeRenderer-prod.js -0.3% -0.4% 245.21 KB 244.39 KB 42.57 KB 42.4 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js -0.3% -0.4% 253.17 KB 252.43 KB 44.22 KB 44.06 KB RN_OSS_PROFILING
ReactFabric-dev.js 0.0% 0.0% 626.23 KB 626.24 KB 133.21 KB 133.22 KB RN_FB_DEV
ReactFabric-prod.js -0.3% -0.4% 238.36 KB 237.55 KB 41.26 KB 41.1 KB RN_FB_PROD
ReactFabric-profiling.js -0.3% -0.4% 246.32 KB 245.58 KB 42.96 KB 42.77 KB RN_FB_PROFILING
ReactFabric-dev.js 0.0% 0.0% 626.13 KB 626.15 KB 133.17 KB 133.18 KB RN_OSS_DEV
ReactFabric-prod.js -0.3% -0.4% 238.36 KB 237.55 KB 41.25 KB 41.1 KB RN_OSS_PROD
ReactFabric-profiling.js -0.3% -0.4% 246.33 KB 245.59 KB 42.95 KB 42.76 KB RN_OSS_PROFILING

Generated by 🚫 dangerJS

@gaearon
Copy link
Collaborator

gaearon commented May 14, 2019

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?

@acdlite
Copy link
Collaborator Author

acdlite commented May 14, 2019

@gaearon In a real test I would simulate a click event. Since ReactNoop doesn't have an event system, I used interactiveUpdates. I don't consider interactiveUpdates a public API but eventually we will need some way to create a custom discrete event handler. I think it will be two functions instead of one: a function to wrap the discrete event handler, and a function to flush pending discrete updates.

@gaearon
Copy link
Collaborator

gaearon commented 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?

@acdlite
Copy link
Collaborator Author

acdlite commented May 15, 2019

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 batchedUpdates or flushSync.

You'd still need a way to force flush passive effects. We could expose unstable_flushPassiveEffects but I'm not confident enough to make that stable.

Regardless, that problem exists independently of the problem fixed by this PR (where flushPassiveEffects gets called after the event handler has already started).

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.
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
The change in #15650 has fully rolled out, so we can remove the flag
that reverts it.
This was referenced Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants