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

Suppress unmounted setState warning after flushing passive effects #15122

Closed
wants to merge 2 commits into from

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Mar 15, 2019

Fixes #15057. (I verified sandbox doesn't warn anymore.)

If setState(foo) caused passive effects to flush, and those in turn caused the component to unmount, there is no way somebody could have foreseen that and guarded setState early. Therefore, flushing passive effects temporarily disables the warning until after the next time the work is scheduled.

@sizebot
Copy link

sizebot commented Mar 15, 2019

ReactDOM: size: 0.0%, gzip: -0.0%

Details of bundled changes.

Comparing: d0289c7...dd4064a

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.1% +0.1% 775.58 KB 776.14 KB 176.45 KB 176.6 KB UMD_DEV
react-dom.production.min.js 0.0% -0.0% 104.89 KB 104.89 KB 34.06 KB 34.06 KB UMD_PROD
react-dom.profiling.min.js 0.0% -0.0% 107.9 KB 107.9 KB 34.72 KB 34.72 KB UMD_PROFILING
react-dom.development.js +0.1% +0.1% 769.97 KB 770.53 KB 174.94 KB 175.09 KB NODE_DEV
react-dom.production.min.js 0.0% -0.0% 104.87 KB 104.87 KB 33.53 KB 33.53 KB NODE_PROD
react-dom.profiling.min.js 0.0% 0.0% 107.99 KB 107.99 KB 34.17 KB 34.17 KB NODE_PROFILING
ReactDOM-dev.js +0.1% +0.1% 792.88 KB 793.45 KB 176.09 KB 176.23 KB FB_WWW_DEV
ReactDOM-profiling.js 0.0% -0.0% 326.93 KB 326.93 KB 60.08 KB 60.08 KB FB_WWW_PROFILING
react-dom-unstable-fire.development.js +0.1% +0.1% 775.92 KB 776.48 KB 176.59 KB 176.74 KB UMD_DEV
react-dom-unstable-fire.production.min.js 0.0% -0.0% 104.91 KB 104.91 KB 34.07 KB 34.07 KB UMD_PROD
react-dom-unstable-fire.profiling.min.js 0.0% -0.0% 107.91 KB 107.91 KB 34.73 KB 34.73 KB UMD_PROFILING
react-dom-unstable-fire.development.js +0.1% +0.1% 770.31 KB 770.87 KB 175.08 KB 175.23 KB NODE_DEV
react-dom-unstable-fire.production.min.js 0.0% -0.0% 104.89 KB 104.89 KB 33.54 KB 33.54 KB NODE_PROD
react-dom-unstable-fire.profiling.min.js 0.0% 0.0% 108 KB 108 KB 34.18 KB 34.18 KB NODE_PROFILING
ReactFire-dev.js +0.1% +0.1% 792.09 KB 792.66 KB 176.05 KB 176.19 KB FB_WWW_DEV
ReactFire-prod.js 0.0% -0.0% 309.53 KB 309.53 KB 56.44 KB 56.43 KB FB_WWW_PROD
react-dom-test-utils.development.js 0.0% -0.0% 47.06 KB 47.06 KB 12.99 KB 12.99 KB UMD_DEV
react-dom-test-utils.production.min.js 0.0% -0.1% 10.27 KB 10.27 KB 3.8 KB 3.8 KB UMD_PROD
react-dom-test-utils.production.min.js 0.0% -0.0% 10.05 KB 10.05 KB 3.73 KB 3.73 KB NODE_PROD
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 60.61 KB 60.61 KB 15.92 KB 15.92 KB UMD_DEV
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 60.28 KB 60.28 KB 15.79 KB 15.79 KB NODE_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.0% 10.75 KB 10.75 KB 3.71 KB 3.71 KB NODE_PROD
react-dom-server.browser.development.js 0.0% -0.0% 127.39 KB 127.39 KB 33.95 KB 33.95 KB UMD_DEV
react-dom-server.browser.production.min.js 0.0% -0.0% 18.89 KB 18.89 KB 7.22 KB 7.22 KB UMD_PROD
ReactDOMServer-prod.js 0.0% -0.0% 45.27 KB 45.27 KB 10.43 KB 10.43 KB FB_WWW_PROD
react-dom-server.node.development.js 0.0% -0.0% 125.58 KB 125.58 KB 33.56 KB 33.56 KB NODE_DEV
react-dom-server.node.production.min.js 0.0% -0.0% 19.69 KB 19.69 KB 7.52 KB 7.52 KB NODE_PROD
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 3.63 KB 3.63 KB 1.44 KB 1.44 KB UMD_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.1% 1.21 KB 1.21 KB 706 B 705 B UMD_PROD
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 3.45 KB 3.45 KB 1.39 KB 1.39 KB NODE_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.3% 1.05 KB 1.05 KB 637 B 635 B NODE_PROD
react-dom-unstable-fizz.node.development.js 0.0% -0.1% 3.7 KB 3.7 KB 1.42 KB 1.42 KB NODE_DEV
react-dom-unstable-fizz.node.production.min.js 0.0% -0.3% 1.1 KB 1.1 KB 668 B 666 B NODE_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.1% +0.1% 551.25 KB 551.81 KB 119.63 KB 119.78 KB UMD_DEV
react-art.production.min.js 0.0% -0.0% 96.91 KB 96.91 KB 29.88 KB 29.87 KB UMD_PROD
react-art.development.js +0.1% +0.1% 482.14 KB 482.7 KB 102.34 KB 102.49 KB NODE_DEV
react-art.production.min.js 0.0% -0.0% 61.83 KB 61.83 KB 18.99 KB 18.98 KB NODE_PROD
ReactART-dev.js +0.1% +0.1% 491.35 KB 491.91 KB 101.58 KB 101.71 KB FB_WWW_DEV
ReactART-prod.js 0.0% -0.0% 194.9 KB 194.9 KB 33.1 KB 33.1 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.1% +0.1% 617.14 KB 617.7 KB 131.86 KB 132.01 KB RN_FB_DEV
ReactNativeRenderer-profiling.js 0.0% -0.0% 251.82 KB 251.82 KB 44.34 KB 44.34 KB RN_FB_PROFILING
ReactNativeRenderer-dev.js +0.1% +0.1% 617.05 KB 617.62 KB 131.82 KB 131.98 KB RN_OSS_DEV
ReactNativeRenderer-profiling.js 0.0% -0.0% 251.84 KB 251.84 KB 44.34 KB 44.34 KB RN_OSS_PROFILING
ReactFabric-dev.js +0.1% +0.1% 607.99 KB 608.55 KB 129.58 KB 129.73 KB RN_FB_DEV
ReactFabric-prod.js 0.0% -0.0% 238.78 KB 238.78 KB 41.52 KB 41.52 KB RN_FB_PROD
ReactFabric-dev.js +0.1% +0.1% 607.9 KB 608.46 KB 129.53 KB 129.68 KB RN_OSS_DEV
ReactFabric-prod.js 0.0% -0.0% 238.78 KB 238.78 KB 41.52 KB 41.51 KB RN_OSS_PROD
ReactFabric-profiling.js 0.0% -0.0% 244.17 KB 244.17 KB 42.87 KB 42.87 KB RN_OSS_PROFILING

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.1% +0.1% 491.18 KB 491.74 KB 104.05 KB 104.19 KB UMD_DEV
react-test-renderer.production.min.js 0.0% -0.0% 62.91 KB 62.91 KB 19.33 KB 19.33 KB UMD_PROD
react-test-renderer.development.js +0.1% +0.1% 486.7 KB 487.26 KB 102.89 KB 103.04 KB NODE_DEV
react-test-renderer.production.min.js 0.0% -0.0% 62.56 KB 62.56 KB 19.07 KB 19.07 KB NODE_PROD
ReactTestRenderer-dev.js +0.1% +0.1% 496.72 KB 497.28 KB 102.52 KB 102.66 KB FB_WWW_DEV
react-test-renderer-shallow.development.js 0.0% -0.0% 37.2 KB 37.2 KB 9.5 KB 9.5 KB UMD_DEV

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.1% +0.1% 480.33 KB 480.89 KB 100.86 KB 101 KB NODE_DEV
react-reconciler.production.min.js 0.0% -0.0% 63 KB 63 KB 18.84 KB 18.84 KB NODE_PROD
react-reconciler-persistent.development.js +0.1% +0.1% 478.53 KB 479.09 KB 100.15 KB 100.29 KB NODE_DEV
react-reconciler-reflection.development.js 0.0% -0.0% 15.76 KB 15.76 KB 4.98 KB 4.98 KB NODE_DEV

Generated by 🚫 dangerJS

@acdlite
Copy link
Collaborator

acdlite commented Mar 16, 2019

I've been thinking about this fix and I'm worried it exposes a deeper flaw about when we flush passive effects. 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 PR/bug is an example of why.

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.

So, I wonder if a better fix would be to remove flushPassiveEffects from setState and instead flush them in the same place we flush serial events. This function:

function flushInteractiveUpdates() {
if (
!isRendering &&
lowestPriorityPendingInteractiveExpirationTime !== NoWork
) {
// Synchronously flush pending interactive updates.
performWork(lowestPriorityPendingInteractiveExpirationTime, false);
lowestPriorityPendingInteractiveExpirationTime = NoWork;
}
}

And then all other updates are assumed to be non-serial, so the order in the queue shouldn't matter.

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Mar 16, 2019

Yea silencing is not the fix here. It surfaces a more fundamental issue that can surface itself with other effects than just setState. Eg imagine that this setState is fine but you also perform another effect such as subscribing/unsubscribing.

@necolas necolas added the React Core Team Opened by a member of the React Core Team label Jan 8, 2020
@gaearon gaearon closed this Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

setState in useEffect causing a "React state update on an unmounted component" warning
6 participants