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

Bugfix: Do not unhide a suspended tree without finishing the suspended update #18411

Merged
merged 2 commits into from
Mar 30, 2020

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Mar 27, 2020

Fixes #18357

When we commit a fallback, we cannot unhide the content without including the level that originally suspended. That's because the work at level outside the boundary (i.e. everything that wasn't hidden during that render) already committed.

When we commit a fallback, we cannot unhide the content without including
the level that originally suspended. That's because the work at level
outside the boundary (i.e. everything that wasn't hidden during that
render) already committed.
@acdlite acdlite requested review from sebmarkbage and gaearon March 27, 2020 22:49
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 27, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 27, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit b225cc1:

Sandbox Source
condescending-morse-re0t6 Configuration
morning-frost-y6cei Issue #18357

@sizebot
Copy link

sizebot commented Mar 27, 2020

Details of bundled changes.

Comparing: 35a2f74...b225cc1

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.production.min.js 🔺+0.2% 🔺+0.2% 118.55 KB 118.79 KB 37.26 KB 37.35 KB NODE_PROD
react-dom-test-utils.development.js 0.0% -0.0% 59.23 KB 59.23 KB 15.64 KB 15.64 KB UMD_DEV
ReactDOMTesting-profiling.js +0.3% +0.4% 406 KB 407.42 KB 73.77 KB 74.03 KB FB_WWW_PROFILING
react-dom-server.browser.production.min.js 0.0% -0.0% 19.62 KB 19.62 KB 7.29 KB 7.29 KB NODE_PROD
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 3.34 KB 3.34 KB 1.26 KB 1.26 KB UMD_DEV
react-dom.profiling.min.js +0.2% +0.3% 122.4 KB 122.65 KB 38.42 KB 38.53 KB NODE_PROFILING
react-dom-test-utils.production.min.js 0.0% -0.1% 12.25 KB 12.25 KB 4.55 KB 4.54 KB UMD_PROD
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.3% 1.17 KB 1.17 KB 692 B 690 B UMD_PROD
react-dom-server.node.development.js 0.0% -0.0% 127.53 KB 127.53 KB 34.09 KB 34.09 KB NODE_DEV
react-dom-server.node.production.min.js 0.0% -0.0% 20.03 KB 20.03 KB 7.44 KB 7.44 KB NODE_PROD
ReactDOMForked-dev.js +0.2% +0.2% 1007.19 KB 1009.17 KB 223.72 KB 224.17 KB FB_WWW_DEV
ReactDOMForked-prod.js 🔺+0.3% 🔺+0.3% 424.76 KB 426.18 KB 76.99 KB 77.25 KB FB_WWW_PROD
react-dom-unstable-fizz.node.development.js 0.0% -0.1% 3.68 KB 3.68 KB 1.33 KB 1.33 KB NODE_DEV
react-dom.development.js +0.2% +0.2% 907.39 KB 909.43 KB 199.48 KB 199.89 KB UMD_DEV
ReactDOMForked-profiling.js +0.3% +0.3% 442.47 KB 443.91 KB 80.06 KB 80.3 KB FB_WWW_PROFILING
react-dom-server.browser.development.js 0.0% -0.0% 133.15 KB 133.15 KB 34.27 KB 34.27 KB UMD_DEV
react-dom-unstable-fizz.node.production.min.js 0.0% -0.3% 1.15 KB 1.15 KB 655 B 653 B NODE_PROD
react-dom.production.min.js 🔺+0.2% 🔺+0.2% 118.47 KB 118.72 KB 38.03 KB 38.12 KB UMD_PROD
react-dom-server.browser.production.min.js 0.0% -0.0% 19.71 KB 19.71 KB 7.34 KB 7.34 KB UMD_PROD
react-dom.profiling.min.js +0.2% +0.3% 122.18 KB 122.43 KB 39.22 KB 39.34 KB UMD_PROFILING
ReactDOMTesting-dev.js +0.2% +0.2% 944.82 KB 946.8 KB 210.8 KB 211.26 KB FB_WWW_DEV
react-dom.development.js +0.2% +0.2% 863.71 KB 865.67 KB 197.04 KB 197.44 KB NODE_DEV
ReactDOMTesting-prod.js 🔺+0.3% 🔺+0.4% 406 KB 407.42 KB 73.77 KB 74.03 KB FB_WWW_PROD
react-dom-server.browser.development.js 0.0% -0.0% 126.32 KB 126.32 KB 33.84 KB 33.84 KB NODE_DEV
ReactDOM-dev.js +0.2% +0.2% 1007.19 KB 1009.17 KB 223.72 KB 224.17 KB FB_WWW_DEV
ReactDOMServer-dev.js 0.0% -0.0% 136.32 KB 136.32 KB 34.74 KB 34.74 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+0.3% 🔺+0.3% 424.76 KB 426.18 KB 76.99 KB 77.25 KB FB_WWW_PROD
react-dom-test-utils.development.js 0.0% -0.0% 54.69 KB 54.69 KB 15.03 KB 15.03 KB NODE_DEV
ReactDOMServer-prod.js 0.0% -0.0% 46.62 KB 46.62 KB 10.86 KB 10.86 KB FB_WWW_PROD
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 2.94 KB 2.94 KB 1.17 KB 1.17 KB NODE_DEV
ReactDOM-profiling.js +0.3% +0.3% 442.47 KB 443.91 KB 80.06 KB 80.3 KB FB_WWW_PROFILING
react-dom-test-utils.production.min.js 0.0% -0.0% 12.1 KB 12.1 KB 4.47 KB 4.47 KB NODE_PROD
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 57.7 KB 57.7 KB 14.36 KB 14.36 KB UMD_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.5% 1013 B 1013 B 604 B 601 B NODE_PROD
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.1% 10.31 KB 10.31 KB 3.49 KB 3.49 KB UMD_PROD
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 54.71 KB 54.71 KB 14.14 KB 14.13 KB NODE_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.1% 10.04 KB 10.04 KB 3.37 KB 3.37 KB NODE_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.3% +0.3% 653.53 KB 655.64 KB 141.26 KB 141.71 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 🔺+0.4% 🔺+0.4% 268.81 KB 269.87 KB 46.63 KB 46.8 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js +0.4% +0.4% 280.65 KB 281.7 KB 48.89 KB 49.07 KB RN_OSS_PROFILING
ReactFabric-dev.js +0.3% +0.3% 635.39 KB 637.5 KB 136.85 KB 137.31 KB RN_OSS_DEV
ReactFabric-prod.js 🔺+0.4% 🔺+0.3% 260.79 KB 261.85 KB 45.15 KB 45.31 KB RN_OSS_PROD
ReactFabric-profiling.js +0.4% +0.4% 272.63 KB 273.69 KB 47.39 KB 47.56 KB RN_OSS_PROFILING

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.4% +0.3% 539.46 KB 541.43 KB 116.17 KB 116.58 KB NODE_DEV
react-art.production.min.js 🔺+0.3% 🔺+0.4% 71.47 KB 71.71 KB 21.57 KB 21.65 KB NODE_PROD
ReactART-dev.js +0.3% +0.4% 605.92 KB 607.88 KB 126.84 KB 127.3 KB FB_WWW_DEV
ReactART-prod.js 🔺+0.6% 🔺+0.5% 243.07 KB 244.42 KB 41.26 KB 41.48 KB FB_WWW_PROD
react-art.development.js +0.3% +0.3% 635.36 KB 637.4 KB 133.81 KB 134.22 KB UMD_DEV
react-art.production.min.js 🔺+0.2% 🔺+0.3% 106.42 KB 106.67 KB 32.3 KB 32.39 KB UMD_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer-shallow.development.js 0.0% -0.0% 38.39 KB 38.39 KB 9.29 KB 9.29 KB UMD_DEV
react-test-renderer-shallow.production.min.js 0.0% -0.0% 12.91 KB 12.91 KB 3.94 KB 3.93 KB UMD_PROD
react-test-renderer.development.js +0.4% +0.3% 546.03 KB 547.99 KB 117.64 KB 118.04 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.3% 🔺+0.4% 73.26 KB 73.5 KB 22.09 KB 22.18 KB NODE_PROD
ReactTestRenderer-dev.js +0.4% +0.4% 578.56 KB 580.68 KB 122.08 KB 122.53 KB FB_WWW_DEV
react-test-renderer.development.js +0.4% +0.3% 572.83 KB 574.87 KB 119.06 KB 119.48 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.3% 🔺+0.3% 73.44 KB 73.69 KB 22.39 KB 22.47 KB UMD_PROD

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.3% +0.3% 577.5 KB 579.46 KB 121.97 KB 122.38 KB NODE_DEV
react-reconciler-reflection.development.js 0.0% -0.0% 15.55 KB 15.55 KB 4.73 KB 4.73 KB NODE_DEV
react-reconciler.production.min.js 🔺+0.3% 🔺+0.4% 76.31 KB 76.55 KB 22.64 KB 22.74 KB NODE_PROD
react-reconciler-reflection.production.min.js 0.0% -0.3% 2.57 KB 2.57 KB 1.09 KB 1.08 KB NODE_PROD

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

Size changes (stable)

Generated by 🚫 dangerJS against b225cc1

@sizebot
Copy link

sizebot commented Mar 27, 2020

Details of bundled changes.

Comparing: 35a2f74...b225cc1

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.4% +0.3% 572.86 KB 574.9 KB 119.08 KB 119.49 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.3% 🔺+0.3% 73.47 KB 73.71 KB 22.41 KB 22.48 KB UMD_PROD
react-test-renderer-shallow.development.js 0.0% -0.0% 38.4 KB 38.4 KB 9.3 KB 9.3 KB UMD_DEV
react-test-renderer-shallow.production.min.js 0.0% -0.0% 12.92 KB 12.92 KB 3.95 KB 3.94 KB UMD_PROD
react-test-renderer.development.js +0.4% +0.3% 546.05 KB 548.02 KB 117.65 KB 118.05 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.3% 🔺+0.4% 73.29 KB 73.53 KB 22.11 KB 22.2 KB NODE_PROD
ReactTestRenderer-dev.js +0.4% +0.4% 578.57 KB 580.69 KB 122.09 KB 122.54 KB FB_WWW_DEV

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom-server.browser.development.js 0.0% -0.0% 134.74 KB 134.74 KB 34.47 KB 34.47 KB UMD_DEV
react-dom-server.browser.production.min.js 0.0% -0.0% 20.17 KB 20.17 KB 7.41 KB 7.4 KB UMD_PROD
react-dom.profiling.min.js +0.2% +0.3% 126.63 KB 126.92 KB 39.63 KB 39.74 KB NODE_PROFILING
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 3.35 KB 3.35 KB 1.27 KB 1.27 KB UMD_DEV
ReactDOM-dev.js +0.2% +0.2% 961.33 KB 963.32 KB 213.68 KB 214.15 KB FB_WWW_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.3% 1.19 KB 1.19 KB 700 B 698 B UMD_PROD
ReactDOM-prod.js 🔺+0.4% 🔺+0.3% 399.08 KB 400.5 KB 72.49 KB 72.71 KB FB_WWW_PROD
ReactDOM-profiling.js +0.3% +0.3% 416.74 KB 418.17 KB 75.49 KB 75.75 KB FB_WWW_PROFILING
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 2.95 KB 2.95 KB 1.18 KB 1.18 KB NODE_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.5% 1 KB 1 KB 612 B 609 B NODE_PROD
react-dom-test-utils.development.js 0.0% -0.0% 59.24 KB 59.24 KB 15.65 KB 15.65 KB UMD_DEV
react-dom-test-utils.production.min.js 0.0% -0.0% 12.26 KB 12.26 KB 4.55 KB 4.55 KB UMD_PROD
ReactDOMTesting-dev.js +0.2% +0.2% 918.06 KB 920.04 KB 204.9 KB 205.35 KB FB_WWW_DEV
ReactDOMTesting-prod.js 🔺+0.4% 🔺+0.3% 393.26 KB 394.68 KB 71.77 KB 71.99 KB FB_WWW_PROD
react-dom-test-utils.development.js 0.0% -0.0% 54.7 KB 54.7 KB 15.04 KB 15.04 KB NODE_DEV
ReactDOMTesting-profiling.js +0.4% +0.3% 393.26 KB 394.68 KB 71.77 KB 71.99 KB FB_WWW_PROFILING
react-dom-server.node.development.js 0.0% -0.0% 129.05 KB 129.05 KB 34.3 KB 34.3 KB NODE_DEV
react-dom-test-utils.production.min.js 0.0% -0.0% 12.12 KB 12.12 KB 4.48 KB 4.48 KB NODE_PROD
react-dom-server.node.production.min.js 0.0% -0.0% 20.49 KB 20.49 KB 7.52 KB 7.52 KB NODE_PROD
react-dom-server.browser.development.js 0.0% -0.0% 127.83 KB 127.83 KB 34.05 KB 34.05 KB NODE_DEV
react-dom-server.browser.production.min.js 0.0% -0.0% 20.08 KB 20.08 KB 7.38 KB 7.38 KB NODE_PROD
react-dom.development.js +0.2% +0.2% 937.54 KB 939.38 KB 204.74 KB 205.17 KB UMD_DEV
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 57.71 KB 57.71 KB 14.37 KB 14.36 KB UMD_DEV
react-dom.production.min.js 🔺+0.2% 🔺+0.3% 122.5 KB 122.79 KB 39.14 KB 39.27 KB UMD_PROD
ReactDOMForked-dev.js +0.2% +0.2% 961.33 KB 963.32 KB 213.68 KB 214.15 KB FB_WWW_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.1% 10.32 KB 10.32 KB 3.5 KB 3.49 KB UMD_PROD
ReactDOMServer-dev.js 0.0% -0.0% 135.47 KB 135.47 KB 34.6 KB 34.6 KB FB_WWW_DEV
react-dom.profiling.min.js +0.2% +0.3% 126.33 KB 126.63 KB 40.42 KB 40.54 KB UMD_PROFILING
ReactDOMForked-prod.js 🔺+0.4% 🔺+0.3% 399.08 KB 400.5 KB 72.49 KB 72.71 KB FB_WWW_PROD
ReactDOMServer-prod.js 0.0% -0.0% 45.91 KB 45.91 KB 10.67 KB 10.67 KB FB_WWW_PROD
react-dom.development.js +0.2% +0.2% 892.61 KB 894.38 KB 202.24 KB 202.67 KB NODE_DEV
ReactDOMForked-profiling.js +0.3% +0.3% 416.74 KB 418.17 KB 75.49 KB 75.75 KB FB_WWW_PROFILING
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 54.72 KB 54.72 KB 14.14 KB 14.14 KB NODE_DEV
react-dom-unstable-fizz.node.development.js 0.0% -0.2% 3.69 KB 3.69 KB 1.34 KB 1.34 KB NODE_DEV
react-dom.production.min.js 🔺+0.2% 🔺+0.3% 122.65 KB 122.94 KB 38.36 KB 38.46 KB NODE_PROD
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.1% 10.05 KB 10.05 KB 3.38 KB 3.38 KB NODE_PROD
react-dom-unstable-fizz.node.production.min.js 0.0% -0.3% 1.16 KB 1.16 KB 664 B 662 B NODE_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactART-dev.js +0.3% +0.4% 577.85 KB 579.8 KB 121.17 KB 121.64 KB FB_WWW_DEV
ReactART-prod.js 🔺+0.6% 🔺+0.6% 234.14 KB 235.5 KB 39.83 KB 40.06 KB FB_WWW_PROD
react-art.development.js +0.3% +0.3% 655.98 KB 657.79 KB 137.51 KB 137.92 KB UMD_DEV
react-art.production.min.js 🔺+0.3% 🔺+0.3% 109 KB 109.29 KB 33.06 KB 33.15 KB UMD_PROD
react-art.development.js +0.3% +0.4% 559.28 KB 561.02 KB 119.89 KB 120.32 KB NODE_DEV
react-art.production.min.js 🔺+0.4% 🔺+0.4% 73.99 KB 74.27 KB 22.21 KB 22.3 KB NODE_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.3% +0.3% 656.25 KB 658.37 KB 141.61 KB 142.06 KB RN_FB_DEV
ReactNativeRenderer-prod.js 🔺+0.4% 🔺+0.4% 268.97 KB 270.03 KB 46.67 KB 46.84 KB RN_FB_PROD
ReactNativeRenderer-profiling.js +0.4% +0.4% 280.81 KB 281.85 KB 48.92 KB 49.11 KB RN_FB_PROFILING
ReactFabric-dev.js +0.3% +0.3% 635.4 KB 637.52 KB 136.86 KB 137.31 KB RN_OSS_DEV
ReactFabric-prod.js 🔺+0.4% 🔺+0.3% 260.81 KB 261.87 KB 45.16 KB 45.32 KB RN_OSS_PROD
ReactFabric-profiling.js +0.4% +0.4% 272.65 KB 273.7 KB 47.4 KB 47.57 KB RN_OSS_PROFILING
ReactFabric-dev.js +0.3% +0.3% 638.12 KB 640.23 KB 137.19 KB 137.64 KB RN_FB_DEV
ReactFabric-prod.js 🔺+0.4% 🔺+0.4% 260.96 KB 262.02 KB 45.19 KB 45.35 KB RN_FB_PROD
ReactNativeRenderer-dev.js +0.3% +0.3% 653.54 KB 655.66 KB 141.26 KB 141.72 KB RN_OSS_DEV
ReactFabric-profiling.js +0.4% +0.4% 272.8 KB 273.85 KB 47.42 KB 47.6 KB RN_FB_PROFILING
ReactNativeRenderer-prod.js 🔺+0.4% 🔺+0.4% 268.82 KB 269.88 KB 46.64 KB 46.8 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js +0.4% +0.4% 280.67 KB 281.71 KB 48.89 KB 49.08 KB RN_OSS_PROFILING

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler-reflection.development.js 0.0% -0.0% 15.56 KB 15.56 KB 4.73 KB 4.73 KB NODE_DEV
react-reconciler-reflection.production.min.js 0.0% -0.2% 2.58 KB 2.58 KB 1.09 KB 1.09 KB NODE_PROD
react-reconciler.development.js +0.3% +0.3% 599.65 KB 601.42 KB 126.2 KB 126.64 KB NODE_DEV
react-reconciler.production.min.js 🔺+0.4% 🔺+0.5% 79.23 KB 79.52 KB 23.33 KB 23.45 KB NODE_PROD

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

Size changes (experimental)

Generated by 🚫 dangerJS against b225cc1

@acdlite acdlite force-pushed the bugfix-flickering branch from da27d9e to b225cc1 Compare March 30, 2020 17:50
@acdlite acdlite merged commit d7382b6 into facebook:master Mar 30, 2020
acdlite added a commit to acdlite/react that referenced this pull request Apr 17, 2020
I think this was just poor factoring on my part in facebook#18411. Honestly it
doesn't make much sense to me, but my best guess is that I must have
though that when `baseTime > currentChildExpirationTime`, the function
would fall through to the
`currentChildExpirationTime < renderExpirationTime` branch below.

Really I think just made an oopsie.

Regardless, this logic is galaxy brainéd. A goal of the Lanes refactor
I'm working on is to make these types of checks -- is there remaining
work in this tree? -- a lot easier to think about. Hopefully.
@acdlite acdlite mentioned this pull request Apr 17, 2020
acdlite added a commit to acdlite/react that referenced this pull request Apr 17, 2020
I think this was just poor factoring on my part in facebook#18411. Honestly it
doesn't make much sense to me, but my best guess is that I must have
thought that when `baseTime > currentChildExpirationTime`, the function
would fall through to the
`currentChildExpirationTime < renderExpirationTime` branch below.

Really I think just made an oopsie.

Regardless, this logic is galaxy brainéd. A goal of the Lanes refactor
I'm working on is to make these types of checks -- is there remaining
work in this tree? -- a lot easier to think about. Hopefully.
acdlite added a commit that referenced this pull request Apr 17, 2020
* Failing test for #18657

* Remove incorrect priority check

I think this was just poor factoring on my part in #18411. Honestly it
doesn't make much sense to me, but my best guess is that I must have
thought that when `baseTime > currentChildExpirationTime`, the function
would fall through to the
`currentChildExpirationTime < renderExpirationTime` branch below.

Really I think just made an oopsie.

Regardless, this logic is galaxy brainéd. A goal of the Lanes refactor
I'm working on is to make these types of checks -- is there remaining
work in this tree? -- a lot easier to think about. Hopefully.
acdlite added a commit to acdlite/react that referenced this pull request Apr 25, 2020
When a Suspense boundary is in its fallback state, you cannot switch
back to the main content without also finishing any updates inside the
tree that might have been skipped. That would be a form of tearing.

Before we fixed this in facebook#18411, the way this bug manfiested was that a
boundary was suspended by an update that originated from a child
component (as opposed to props from a parent). While the fallback was
showing, it received another update, this time at high priority. React
would render the high priority update without also including the
original update. That would cause the fallback to switch back to the
main content, since the update that caused the tree to suspend was no
longer part of the render. But then, React would immediately try to
render the original update, which would again suspend and show the
fallback, leading to a momentary flicker in the UI.

The approach added in facebook#18411 is, when receiving a high priority update
to a Suspense tree that's in its fallback state is to bail out, keep
showing the fallback and finish the update in the rest of the tree.
After that commits, render again at the original priority. Because low
priority expiration times are inclusive of higher priority expiration
times, this ensures that all the updates are committed together.

The new approach in this commit is to turn `renderExpirationTime` into a
context-like value that lives on the stack. Then, when unhiding the
Suspense boundary, we can push a new `renderExpirationTime` that is
inclusive of both the high pri update and the original update that
suspended. Then the boundary can be unblocked in a single render pass.

An advantage of the old approach is that by deferring the work of
unhiding, there's less work to do in the high priority update.

The key advantage of the new approach is that it solves the consistency
problem without having to entangle the entire root.
acdlite added a commit to acdlite/react that referenced this pull request Apr 25, 2020
When a Suspense boundary is in its fallback state, you cannot switch
back to the main content without also finishing any updates inside the
tree that might have been skipped. That would be a form of tearing.

Before we fixed this in facebook#18411, the way this bug manifested was that a
boundary was suspended by an update that originated from a child
component (as opposed to props from a parent). While the fallback was
showing, it received another update, this time at high priority. React
would render the high priority update without also including the
original update. That would cause the fallback to switch back to the
main content, since the update that caused the tree to suspend was no
longer part of the render. But then, React would immediately try to
render the original update, which would again suspend and show the
fallback, leading to a momentary flicker in the UI.

The approach added in facebook#18411 is, when receiving a high priority update
to a Suspense tree that's in its fallback state is to bail out, keep
showing the fallback and finish the update in the rest of the tree.
After that commits, render again at the original priority. Because low
priority expiration times are inclusive of higher priority expiration
times, this ensures that all the updates are committed together.

The new approach in this commit is to turn `renderExpirationTime` into a
context-like value that lives on the stack. Then, when unhiding the
Suspense boundary, we can push a new `renderExpirationTime` that is
inclusive of both the high pri update and the original update that
suspended. Then the boundary can be unblocked in a single render pass.

An advantage of the old approach is that by deferring the work of
unhiding, there's less work to do in the high priority update.

The key advantage of the new approach is that it solves the consistency
problem without having to entangle the entire root.
acdlite added a commit to acdlite/react that referenced this pull request Apr 25, 2020
When a Suspense boundary is in its fallback state, you cannot switch
back to the main content without also finishing any updates inside the
tree that might have been skipped. That would be a form of tearing.

Before we fixed this in facebook#18411, the way this bug manifested was that a
boundary was suspended by an update that originated from a child
component (as opposed to props from a parent). While the fallback was
showing, it received another update, this time at high priority. React
would render the high priority update without also including the
original update. That would cause the fallback to switch back to the
main content, since the update that caused the tree to suspend was no
longer part of the render. But then, React would immediately try to
render the original update, which would again suspend and show the
fallback, leading to a momentary flicker in the UI.

The approach added in facebook#18411 is, when receiving a high priority update
to a Suspense tree that's in its fallback state is to bail out, keep
showing the fallback and finish the update in the rest of the tree.
After that commits, render again at the original priority. Because low
priority expiration times are inclusive of higher priority expiration
times, this ensures that all the updates are committed together.

The new approach in this commit is to turn `renderExpirationTime` into a
context-like value that lives on the stack. Then, when unhiding the
Suspense boundary, we can push a new `renderExpirationTime` that is
inclusive of both the high pri update and the original update that
suspended. Then the boundary can be unblocked in a single render pass.

An advantage of the old approach is that by deferring the work of
unhiding, there's less work to do in the high priority update.

The key advantage of the new approach is that it solves the consistency
problem without having to entangle the entire root.
acdlite added a commit to acdlite/react that referenced this pull request Apr 29, 2020
When a Suspense boundary is in its fallback state, you cannot switch
back to the main content without also finishing any updates inside the
tree that might have been skipped. That would be a form of tearing.

Before we fixed this in facebook#18411, the way this bug manifested was that a
boundary was suspended by an update that originated from a child
component (as opposed to props from a parent). While the fallback was
showing, it received another update, this time at high priority. React
would render the high priority update without also including the
original update. That would cause the fallback to switch back to the
main content, since the update that caused the tree to suspend was no
longer part of the render. But then, React would immediately try to
render the original update, which would again suspend and show the
fallback, leading to a momentary flicker in the UI.

The approach added in facebook#18411 is, when receiving a high priority update
to a Suspense tree that's in its fallback state is to bail out, keep
showing the fallback and finish the update in the rest of the tree.
After that commits, render again at the original priority. Because low
priority expiration times are inclusive of higher priority expiration
times, this ensures that all the updates are committed together.

The new approach in this commit is to turn `renderExpirationTime` into a
context-like value that lives on the stack. Then, when unhiding the
Suspense boundary, we can push a new `renderExpirationTime` that is
inclusive of both the high pri update and the original update that
suspended. Then the boundary can be unblocked in a single render pass.

An advantage of the old approach is that by deferring the work of
unhiding, there's less work to do in the high priority update.

The key advantage of the new approach is that it solves the consistency
problem without having to entangle the entire root.
acdlite added a commit to acdlite/react that referenced this pull request May 1, 2020
When a Suspense boundary is in its fallback state, you cannot switch
back to the main content without also finishing any updates inside the
tree that might have been skipped. That would be a form of tearing.

Before we fixed this in facebook#18411, the way this bug manifested was that a
boundary was suspended by an update that originated from a child
component (as opposed to props from a parent). While the fallback was
showing, it received another update, this time at high priority. React
would render the high priority update without also including the
original update. That would cause the fallback to switch back to the
main content, since the update that caused the tree to suspend was no
longer part of the render. But then, React would immediately try to
render the original update, which would again suspend and show the
fallback, leading to a momentary flicker in the UI.

The approach added in facebook#18411 is, when receiving a high priority update
to a Suspense tree that's in its fallback state is to bail out, keep
showing the fallback and finish the update in the rest of the tree.
After that commits, render again at the original priority. Because low
priority expiration times are inclusive of higher priority expiration
times, this ensures that all the updates are committed together.

The new approach in this commit is to turn `renderExpirationTime` into a
context-like value that lives on the stack. Then, when unhiding the
Suspense boundary, we can push a new `renderExpirationTime` that is
inclusive of both the high pri update and the original update that
suspended. Then the boundary can be unblocked in a single render pass.

An advantage of the old approach is that by deferring the work of
unhiding, there's less work to do in the high priority update.

The key advantage of the new approach is that it solves the consistency
problem without having to entangle the entire root.
acdlite added a commit that referenced this pull request May 1, 2020
* Unhide Suspense trees without entanglement

When a Suspense boundary is in its fallback state, you cannot switch
back to the main content without also finishing any updates inside the
tree that might have been skipped. That would be a form of tearing.

Before we fixed this in #18411, the way this bug manifested was that a
boundary was suspended by an update that originated from a child
component (as opposed to props from a parent). While the fallback was
showing, it received another update, this time at high priority. React
would render the high priority update without also including the
original update. That would cause the fallback to switch back to the
main content, since the update that caused the tree to suspend was no
longer part of the render. But then, React would immediately try to
render the original update, which would again suspend and show the
fallback, leading to a momentary flicker in the UI.

The approach added in #18411 is, when receiving a high priority update
to a Suspense tree that's in its fallback state is to bail out, keep
showing the fallback and finish the update in the rest of the tree.
After that commits, render again at the original priority. Because low
priority expiration times are inclusive of higher priority expiration
times, this ensures that all the updates are committed together.

The new approach in this commit is to turn `renderExpirationTime` into a
context-like value that lives on the stack. Then, when unhiding the
Suspense boundary, we can push a new `renderExpirationTime` that is
inclusive of both the high pri update and the original update that
suspended. Then the boundary can be unblocked in a single render pass.

An advantage of the old approach is that by deferring the work of
unhiding, there's less work to do in the high priority update.

The key advantage of the new approach is that it solves the consistency
problem without having to entangle the entire root.

* Create internal LegacyHidden type

This only exists so we can clean up the internal implementation of
`<div hidden={isHidden} />`, which is not a stable feature. The goal
is to move everything to the new Offscreen type instead. However,
Offscreen has different semantics, so before we can remove the legacy
API, we have to migrate our internal usage at Facebook. So we'll need
to maintain both temporarily.

In this initial commit, I've only added the type. It's not used
anywhere. The next step is to use it to implement `hidden`.

* Use LegacyHidden to implement old hidden API

If a host component receives a `hidden` prop, we wrap its children in
an Offscreen fiber. This is similar to what we do for Suspense children.

The LegacyHidden type happens to share the same implementation as the
new Offscreen type, for now, but using separate types allows us to fork
the behavior later when we implement our planned changes to the
Offscreen API.

There are two subtle semantic changes here. One is that the children of
the host component will have their visibility toggled using the same
mechanism we use for Offscreen and Suspense: find the nearest host node
children and give them a style of `display: none`. We didn't used to do
this in the old API, because the `hidden` DOM attribute on the parent
already hides them. So with this change, we're actually "overhiding" the
children. I considered addressing this, but I figure I'll leave it as-is
in case we want to expose the LegacyHidden component type temporarily
to ease migration of Facebook's internal callers to the Offscreen type.

The other subtle semantic change is that, because of the extra fiber
that wraps around the children, this pattern will cause the children
to lose state:

```js
return isHidden ? <div hidden={true} /> : <div />;
```

The reason is that I didn't want to wrap every single host component
in an extra fiber. So I only wrap them if a `hidden` prop exists. In
the above example, that means the children are conditionally wrapped
in an extra fiber, so they don't line up during reconciliation, so
they get remounted every time `isHidden` changes.

The fix is to rewrite to:

```js
return <div hidden={isHidden} />;
```

I don't anticipate this will be a problem at Facebook, especially since
we're only supposed to use `hidden` via a userspace wrapper component.
(And since the bad pattern isn't very React-y, anyway.)

Again, the eventual goal is to delete this completely and replace it
with Offscreen.
acdlite added a commit to acdlite/react that referenced this pull request Jun 7, 2022
This fixes a bug I discovered related to revealing a hidden Offscreen
tree. When this happens, we include in that render all the updates that
had previously been deferred — that is, all the updates that would have
already committed if the tree weren't hidden. This is necessary to avoid
tearing with the surrounding contents. (This was the "flickering"
Suspense bug we found a few years ago: facebook#18411.)

The way we do this is by tracking the lanes of the updates that were
deferred by a hidden tree. These are the "base" lanes. Then, in order
to reveal the hidden tree, we process any update that matches one of
those base lanes.

The bug I discovered is that some of these base lanes may include
updates that were not present at the time the tree was hidden. We cannot
flush those updates earlier that the surrounding contents — that, too,
could cause tearing.

The crux of the problem is that we sometimes reuse the same lane for
base updates and for non-base updates. So the lane alone isn't
sufficient to distinguish between these cases. We must track this in
some other way.

The solution I landed upon was to add an extra OffscreenLane bit to any
update that is made to a hidden tree. Then later when we reveal the
tree, we'll know not to treat them as base updates.

The extra OffscreenLane bit is removed as soon as that lane is committed
by the root (markRootFinished) — at that point, it gets
"upgraded" to a base update.

The trickiest part of this algorithm is reliably detecting when an
update is made to a hidden tree. What makes this challenging is when the
update is received during a concurrent event, while a render is already
in progress — it's possible the work-in-progress render is about to
flip the visibility of the tree that's being updated, leading to a race
condition.

To avoid a race condition, we will wait to read the visibility of the
tree until the current render has finished. In other words, this makes
it an atomic operation. Most of this logic was already implemented
in facebook#24663.

Because this bugfix depends on a moderately risky refactor to the update
queue (facebook#24663), it only works in the "new" reconciler fork. We will roll
it out gradually to www before landing in the main fork.
acdlite added a commit to acdlite/react that referenced this pull request Jun 7, 2022
This fixes a bug I discovered related to revealing a hidden Offscreen
tree. When this happens, we include in that render all the updates that
had previously been deferred — that is, all the updates that would have
already committed if the tree weren't hidden. This is necessary to avoid
tearing with the surrounding contents. (This was the "flickering"
Suspense bug we found a few years ago: facebook#18411.)

The way we do this is by tracking the lanes of the updates that were
deferred by a hidden tree. These are the "base" lanes. Then, in order
to reveal the hidden tree, we process any update that matches one of
those base lanes.

The bug I discovered is that some of these base lanes may include
updates that were not present at the time the tree was hidden. We cannot
flush those updates earlier that the surrounding contents — that, too,
could cause tearing.

The crux of the problem is that we sometimes reuse the same lane for
base updates and for non-base updates. So the lane alone isn't
sufficient to distinguish between these cases. We must track this in
some other way.

The solution I landed upon was to add an extra OffscreenLane bit to any
update that is made to a hidden tree. Then later when we reveal the
tree, we'll know not to treat them as base updates.

The extra OffscreenLane bit is removed as soon as that lane is committed
by the root (markRootFinished) — at that point, it gets
"upgraded" to a base update.

The trickiest part of this algorithm is reliably detecting when an
update is made to a hidden tree. What makes this challenging is when the
update is received during a concurrent event, while a render is already
in progress — it's possible the work-in-progress render is about to
flip the visibility of the tree that's being updated, leading to a race
condition.

To avoid a race condition, we will wait to read the visibility of the
tree until the current render has finished. In other words, this makes
it an atomic operation. Most of this logic was already implemented
in facebook#24663.

Because this bugfix depends on a moderately risky refactor to the update
queue (facebook#24663), it only works in the "new" reconciler fork. We will roll
it out gradually to www before landing in the main fork.
acdlite added a commit to acdlite/react that referenced this pull request Jun 7, 2022
This fixes a bug I discovered related to revealing a hidden Offscreen
tree. When this happens, we include in that render all the updates that
had previously been deferred — that is, all the updates that would have
already committed if the tree weren't hidden. This is necessary to avoid
tearing with the surrounding contents. (This was the "flickering"
Suspense bug we found a few years ago: facebook#18411.)

The way we do this is by tracking the lanes of the updates that were
deferred by a hidden tree. These are the "base" lanes. Then, in order
to reveal the hidden tree, we process any update that matches one of
those base lanes.

The bug I discovered is that some of these base lanes may include
updates that were not present at the time the tree was hidden. We cannot
flush those updates earlier that the surrounding contents — that, too,
could cause tearing.

The crux of the problem is that we sometimes reuse the same lane for
base updates and for non-base updates. So the lane alone isn't
sufficient to distinguish between these cases. We must track this in
some other way.

The solution I landed upon was to add an extra OffscreenLane bit to any
update that is made to a hidden tree. Then later when we reveal the
tree, we'll know not to treat them as base updates.

The extra OffscreenLane bit is removed as soon as that lane is committed
by the root (markRootFinished) — at that point, it gets
"upgraded" to a base update.

The trickiest part of this algorithm is reliably detecting when an
update is made to a hidden tree. What makes this challenging is when the
update is received during a concurrent event, while a render is already
in progress — it's possible the work-in-progress render is about to
flip the visibility of the tree that's being updated, leading to a race
condition.

To avoid a race condition, we will wait to read the visibility of the
tree until the current render has finished. In other words, this makes
it an atomic operation. Most of this logic was already implemented
in facebook#24663.

Because this bugfix depends on a moderately risky refactor to the update
queue (facebook#24663), it only works in the "new" reconciler fork. We will roll
it out gradually to www before landing in the main fork.
acdlite added a commit to acdlite/react that referenced this pull request Jun 7, 2022
This fixes a bug I discovered related to revealing a hidden Offscreen
tree. When this happens, we include in that render all the updates that
had previously been deferred — that is, all the updates that would have
already committed if the tree weren't hidden. This is necessary to avoid
tearing with the surrounding contents. (This was the "flickering"
Suspense bug we found a few years ago: facebook#18411.)

The way we do this is by tracking the lanes of the updates that were
deferred by a hidden tree. These are the "base" lanes. Then, in order
to reveal the hidden tree, we process any update that matches one of
those base lanes.

The bug I discovered is that some of these base lanes may include
updates that were not present at the time the tree was hidden. We cannot
flush those updates earlier that the surrounding contents — that, too,
could cause tearing.

The crux of the problem is that we sometimes reuse the same lane for
base updates and for non-base updates. So the lane alone isn't
sufficient to distinguish between these cases. We must track this in
some other way.

The solution I landed upon was to add an extra OffscreenLane bit to any
update that is made to a hidden tree. Then later when we reveal the
tree, we'll know not to treat them as base updates.

The extra OffscreenLane bit is removed as soon as that lane is committed
by the root (markRootFinished) — at that point, it gets
"upgraded" to a base update.

The trickiest part of this algorithm is reliably detecting when an
update is made to a hidden tree. What makes this challenging is when the
update is received during a concurrent event, while a render is already
in progress — it's possible the work-in-progress render is about to
flip the visibility of the tree that's being updated, leading to a race
condition.

To avoid a race condition, we will wait to read the visibility of the
tree until the current render has finished. In other words, this makes
it an atomic operation. Most of this logic was already implemented
in facebook#24663.

Because this bugfix depends on a moderately risky refactor to the update
queue (facebook#24663), it only works in the "new" reconciler fork. We will roll
it out gradually to www before landing in the main fork.
acdlite added a commit that referenced this pull request Jun 8, 2022
* Add `isHidden` to OffscreenInstance

We need to be able to read whether an offscreen tree is hidden from
an imperative event. We can store this on its OffscreenInstance.

We were already scheduling a commit effect whenever the visibility
changes, in order to toggle the inner effects. So we can reuse that.

* [FORKED] Bugfix: Revealing a hidden update

This fixes a bug I discovered related to revealing a hidden Offscreen
tree. When this happens, we include in that render all the updates that
had previously been deferred — that is, all the updates that would have
already committed if the tree weren't hidden. This is necessary to avoid
tearing with the surrounding contents. (This was the "flickering"
Suspense bug we found a few years ago: #18411.)

The way we do this is by tracking the lanes of the updates that were
deferred by a hidden tree. These are the "base" lanes. Then, in order
to reveal the hidden tree, we process any update that matches one of
those base lanes.

The bug I discovered is that some of these base lanes may include
updates that were not present at the time the tree was hidden. We cannot
flush those updates earlier that the surrounding contents — that, too,
could cause tearing.

The crux of the problem is that we sometimes reuse the same lane for
base updates and for non-base updates. So the lane alone isn't
sufficient to distinguish between these cases. We must track this in
some other way.

The solution I landed upon was to add an extra OffscreenLane bit to any
update that is made to a hidden tree. Then later when we reveal the
tree, we'll know not to treat them as base updates.

The extra OffscreenLane bit is removed as soon as that lane is committed
by the root (markRootFinished) — at that point, it gets
"upgraded" to a base update.

The trickiest part of this algorithm is reliably detecting when an
update is made to a hidden tree. What makes this challenging is when the
update is received during a concurrent event, while a render is already
in progress — it's possible the work-in-progress render is about to
flip the visibility of the tree that's being updated, leading to a race
condition.

To avoid a race condition, we will wait to read the visibility of the
tree until the current render has finished. In other words, this makes
it an atomic operation. Most of this logic was already implemented
in #24663.

Because this bugfix depends on a moderately risky refactor to the update
queue (#24663), it only works in the "new" reconciler fork. We will roll
it out gradually to www before landing in the main fork.

* Add previous commit to list of forked revisions
acdlite added a commit to acdlite/react that referenced this pull request Dec 9, 2022
This code was originally added in the old ExpirationTime implementation
of Suspense. The idea is that if multiple updates suspend inside the
same Suspense boundary, and both of them resolve, we should render both
results in the same batch, to reduce jank.

This was an incomplete idea, though. We later discovered a stronger
requirement — once we show a fallback, we cannot fill in that fallback
without completing _all_ the updates that were previously skipped over.
Otherwise you get tearing. This was fixed by facebook#18411, then we
discovered additional related flaws that were addressed in facebook#24685. See 
those PR descriptions for additional context.

So I believe this older code is no longer necessary.
acdlite added a commit to acdlite/react that referenced this pull request Dec 22, 2022
This code was originally added in the old ExpirationTime implementation
of Suspense. The idea is that if multiple updates suspend inside the
same Suspense boundary, and both of them resolve, we should render both
results in the same batch, to reduce jank.

This was an incomplete idea, though. We later discovered a stronger
requirement — once we show a fallback, we cannot fill in that fallback
without completing _all_ the updates that were previously skipped over.
Otherwise you get tearing. This was fixed by facebook#18411, then we
discovered additional related flaws that were addressed in facebook#24685. See 
those PR descriptions for additional context.

So I believe this older code is no longer necessary.
acdlite added a commit to acdlite/react that referenced this pull request Jan 4, 2023
This code was originally added in the old ExpirationTime implementation
of Suspense. The idea is that if multiple updates suspend inside the
same Suspense boundary, and both of them resolve, we should render both
results in the same batch, to reduce jank.

This was an incomplete idea, though. We later discovered a stronger
requirement — once we show a fallback, we cannot fill in that fallback
without completing _all_ the updates that were previously skipped over.
Otherwise you get tearing. This was fixed by facebook#18411, then we
discovered additional related flaws that were addressed in facebook#24685. See 
those PR descriptions for additional context.

So I believe this older code is no longer necessary.
acdlite added a commit that referenced this pull request Jan 4, 2023
This code was originally added in the old ExpirationTime implementation
of Suspense. The idea is that if multiple updates suspend inside the
same Suspense boundary, and both of them resolve, we should render both
results in the same batch, to reduce jank.

This was an incomplete idea, though. We later discovered a stronger
requirement — once we show a fallback, we cannot fill in that fallback
without completing _all_ the updates that were previously skipped over.
Otherwise you get tearing. This was fixed by #18411, then we discovered
additional related flaws that were addressed in #24685. See those PR
descriptions for additional context.

So I believe this older code is no longer necessary.
github-actions bot pushed a commit that referenced this pull request Jan 4, 2023
This code was originally added in the old ExpirationTime implementation
of Suspense. The idea is that if multiple updates suspend inside the
same Suspense boundary, and both of them resolve, we should render both
results in the same batch, to reduce jank.

This was an incomplete idea, though. We later discovered a stronger
requirement — once we show a fallback, we cannot fill in that fallback
without completing _all_ the updates that were previously skipped over.
Otherwise you get tearing. This was fixed by #18411, then we discovered
additional related flaws that were addressed in #24685. See those PR
descriptions for additional context.

So I believe this older code is no longer necessary.

DiffTrain build for [48274a4](48274a4)
[View git log for this commit](https://github.com/facebook/react/commits/48274a43aa708f63a7580142a4c1c1a47f31c1ac)
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.

Bug: High-pri setState causes primary tree to get unhidden
4 participants