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

Do not add additional work to a batch that is already rendering #13072

Merged
merged 2 commits into from
Jun 19, 2018

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Jun 18, 2018

Depends on #13071

Otherwise, the part of the tree that hasn't rendered yet will receive the latest state, but the already rendered part will show the state as it was before the intervening update.

@pull-bot
Copy link

pull-bot commented Jun 18, 2018

ReactDOM: size: 🔺+0.1%, gzip: 🔺+0.1%

Details of bundled changes.

Comparing: 4fe6eec...4be858d

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.2% +0.1% 625.07 KB 626.27 KB 146.13 KB 146.33 KB UMD_DEV
react-dom.production.min.js 🔺+0.1% 🔺+0.1% 95.27 KB 95.33 KB 30.8 KB 30.82 KB UMD_PROD
react-dom.development.js +0.2% +0.1% 621.2 KB 622.4 KB 144.96 KB 145.15 KB NODE_DEV
react-dom.production.min.js 🔺+0.1% 🔺+0.1% 95.26 KB 95.32 KB 30.33 KB 30.35 KB NODE_PROD
ReactDOM-dev.js +0.2% +0.2% 628.43 KB 629.62 KB 143.56 KB 143.78 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+0.1% 0.0% 272.16 KB 272.35 KB 51.27 KB 51.29 KB FB_WWW_PROD
react-dom.profiling.min.js +0.1% +0.1% 96.16 KB 96.22 KB 30.63 KB 30.65 KB NODE_PROFILING

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.3% +0.2% 409.36 KB 410.55 KB 91.72 KB 91.94 KB UMD_DEV
react-art.production.min.js 🔺+0.1% 0.0% 82.48 KB 82.54 KB 25.4 KB 25.41 KB UMD_PROD
react-art.development.js +0.4% +0.3% 341.33 KB 342.53 KB 74.59 KB 74.81 KB NODE_DEV
react-art.production.min.js 🔺+0.1% 🔺+0.1% 47.46 KB 47.52 KB 14.78 KB 14.8 KB NODE_PROD
ReactART-dev.js +0.4% +0.3% 331.37 KB 332.56 KB 69.69 KB 69.89 KB FB_WWW_DEV
ReactART-prod.js 🔺+0.2% 🔺+0.2% 143.17 KB 143.43 KB 24.61 KB 24.65 KB FB_WWW_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.3% +0.3% 343.6 KB 344.8 KB 74.51 KB 74.75 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.1% 🔺+0.2% 47.38 KB 47.44 KB 14.53 KB 14.55 KB UMD_PROD
react-test-renderer.development.js +0.4% +0.3% 339.72 KB 340.92 KB 73.54 KB 73.79 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.1% 🔺+0.1% 47.08 KB 47.14 KB 14.35 KB 14.37 KB NODE_PROD
ReactTestRenderer-dev.js +0.3% +0.3% 342.88 KB 344.08 KB 72.35 KB 72.54 KB FB_WWW_DEV

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.4% +0.3% 331.85 KB 333.05 KB 71.08 KB 71.28 KB NODE_DEV
react-reconciler.production.min.js 🔺+0.1% 🔺+0.3% 47.19 KB 47.24 KB 14.13 KB 14.17 KB NODE_PROD
react-reconciler-persistent.development.js +0.4% +0.3% 330.47 KB 331.66 KB 70.51 KB 70.71 KB NODE_DEV
react-reconciler-persistent.production.min.js 🔺+0.1% 🔺+0.3% 47.2 KB 47.25 KB 14.14 KB 14.17 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.2% 465.56 KB 466.76 KB 102.49 KB 102.7 KB RN_FB_DEV
ReactNativeRenderer-prod.js 🔺+0.1% 🔺+0.2% 207.19 KB 207.42 KB 36.47 KB 36.53 KB RN_FB_PROD
ReactNativeRenderer-dev.js +0.3% +0.2% 465.27 KB 466.47 KB 102.43 KB 102.64 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 🔺+0.1% 🔺+0.1% 199.75 KB 199.98 KB 35.02 KB 35.06 KB RN_OSS_PROD
ReactFabric-dev.js +0.3% +0.2% 455.81 KB 457.01 KB 100.09 KB 100.3 KB RN_FB_DEV
ReactFabric-prod.js 🔺+0.1% 🔺+0.1% 192.01 KB 192.24 KB 33.61 KB 33.64 KB RN_FB_PROD
ReactFabric-dev.js +0.3% +0.2% 455.85 KB 457.04 KB 100.1 KB 100.32 KB RN_OSS_DEV
ReactFabric-prod.js 🔺+0.1% 🔺+0.1% 192.04 KB 192.28 KB 33.62 KB 33.66 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js +0.1% +0.1% 202.29 KB 202.52 KB 35.57 KB 35.61 KB RN_OSS_PROFILING
ReactFabric-profiling.js +0.1% +0.1% 194.26 KB 194.49 KB 34.09 KB 34.13 KB RN_OSS_PROFILING

Generated by 🚫 dangerJS

@@ -1086,6 +1086,8 @@ function renderRoot(root: FiberRoot, isYieldy: boolean): void {
if (__DEV__) {
resetStackAfterFatalErrorInDev();
}
// Clear the currently rendering root.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These comments are not helpful. Obviously you're clearing the root based on the next line. Say why, not what you're doing.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E.g. "By clearing the root, we force React to start over by rendering the root from scratch instead of continuing in the middle of the tree. We have to do that because we've added new work to the tree and that might otherwise be inconsistent depending on where we are in the current render."

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok Seb, I know what a comment is. Thanks for the explanation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Better comment, please" would have sufficed

@acdlite acdlite force-pushed the dontaddtoalreadyrenderingbatch branch from 92e981c to dfcbfa7 Compare June 19, 2018 17:33
Otherwise, the part of the tree that hasn't rendered yet will receive
the latest state, but the already rendered part will show the state
as it was before the intervening update.
@acdlite acdlite force-pushed the dontaddtoalreadyrenderingbatch branch from dfcbfa7 to 4be858d Compare June 19, 2018 17:35
@acdlite acdlite merged commit 97af3e1 into facebook:master Jun 19, 2018
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

4 participants