Skip to content

Handle legacy root contains in clearContainer #18781

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Handle legacy root contains in clearContainer #18781

wants to merge 2 commits into from

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Apr 29, 2020 •
edited
Loading

When debugging an internal issue, I noticed that we have an issue where we clear down a root that has existing content. This particular fix checks in clearContainer if the container we're trying to clear has a legacy container and if it does, checks to see if it matches the root we expected.

@trueadm trueadm requested a review from bvaughn April 29, 2020 18:57
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 29, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 29, 2020 •
edited
Loading

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 4ba1b2f:

Sandbox Source
strange-wave-3n6h2 Configuration

bvaughn
bvaughn reviewed Apr 29, 2020
View reviewed changes
@gaearon
Copy link
Collaborator

gaearon commented Apr 29, 2020

Btw don't forget to update both files.

@sizebot
Copy link

sizebot commented Apr 29, 2020 •
edited
Loading

Details of bundled changes.

Comparing: 88d0be6...4ba1b2f

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.production.min.js 0.0% 🔺+0.1% 119.98 KB 120.04 KB 37.63 KB 37.65 KB NODE_PROD
ReactDOMTesting-profiling.js 0.0% +0.1% 390.42 KB 390.56 KB 71.13 KB 71.18 KB FB_WWW_PROFILING
react-dom.profiling.min.js 0.0% +0.1% 123.88 KB 123.93 KB 38.79 KB 38.81 KB NODE_PROFILING
react-dom-unstable-fizz.browser.development.js 0.0% +0.1% 4.75 KB 4.75 KB 1.66 KB 1.67 KB NODE_DEV
ReactDOMForked-dev.js 0.0% 0.0% 1.01 MB 1.01 MB 230.19 KB 230.26 KB FB_WWW_DEV
ReactDOMForked-prod.js 0.0% 🔺+0.1% 434.01 KB 434.15 KB 76.02 KB 76.06 KB FB_WWW_PROD
react-dom.development.js 0.0% 0.0% 907.74 KB 908.04 KB 199.63 KB 199.7 KB UMD_DEV
ReactDOMForked-profiling.js 0.0% 0.0% 444.42 KB 444.56 KB 77.77 KB 77.8 KB FB_WWW_PROFILING
react-dom.production.min.js 0.0% 🔺+0.1% 119.86 KB 119.92 KB 38.42 KB 38.45 KB UMD_PROD
react-dom.profiling.min.js 0.0% 0.0% 123.63 KB 123.69 KB 39.62 KB 39.63 KB UMD_PROFILING
ReactDOMTesting-dev.js 0.0% 0.0% 935.34 KB 935.64 KB 208.6 KB 208.69 KB FB_WWW_DEV
react-dom.development.js 0.0% 0.0% 864.05 KB 864.34 KB 197.09 KB 197.17 KB NODE_DEV
ReactDOMTesting-prod.js 0.0% 🔺+0.1% 390.42 KB 390.56 KB 71.13 KB 71.18 KB FB_WWW_PROD
react-dom-server.node.development.js 0.0% -0.0% 146.93 KB 146.93 KB 38.95 KB 38.95 KB NODE_DEV
ReactDOM-dev.js 0.0% 0.0% 1.01 MB 1.02 MB 231.53 KB 231.61 KB FB_WWW_DEV
ReactDOM-prod.js 0.0% 🔺+0.1% 432.68 KB 432.82 KB 75.87 KB 75.92 KB FB_WWW_PROD
react-dom-unstable-fizz.node.development.js 0.0% +0.1% 5.5 KB 5.5 KB 1.82 KB 1.82 KB NODE_DEV
ReactDOM-profiling.js 0.0% 0.0% 443.64 KB 443.78 KB 77.72 KB 77.76 KB FB_WWW_PROFILING
react-dom-test-utils.production.min.js 0.0% 0.0% 13.1 KB 13.1 KB 4.8 KB 4.8 KB NODE_PROD
react-dom-unstable-fizz.node.production.min.js 0.0% 🔺+0.2% 1.16 KB 1.16 KB 658 B 659 B NODE_PROD
react-dom-server.browser.production.min.js 0.0% -0.0% 22.94 KB 22.94 KB 8.52 KB 8.52 KB UMD_PROD
react-dom-server.browser.development.js 0.0% -0.0% 145.71 KB 145.71 KB 38.7 KB 38.69 KB NODE_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.1% 1.19 KB 1.19 KB 696 B 697 B UMD_PROD

Size changes (stable)

Generated by 🚫 dangerJS against 4ba1b2f

@trueadm trueadm closed this Apr 29, 2020
@trueadm trueadm deleted the fix-root-issue branch April 29, 2020 19:03
@sizebot
Copy link

sizebot commented Apr 29, 2020 •
edited
Loading

Details of bundled changes.

Comparing: 88d0be6...4ba1b2f

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.profiling.min.js 0.0% 0.0% 128.91 KB 128.97 KB 40.31 KB 40.32 KB NODE_PROFILING
ReactDOM-dev.js 0.0% 0.0% 1013.64 KB 1013.95 KB 225.98 KB 226.06 KB FB_WWW_DEV
ReactDOM-prod.js 0.0% 🔺+0.1% 421.21 KB 421.36 KB 73.87 KB 73.91 KB FB_WWW_PROD
ReactDOM-profiling.js 0.0% +0.1% 432.11 KB 432.25 KB 75.7 KB 75.74 KB FB_WWW_PROFILING
react-dom-unstable-fizz.node.development.js 0.0% +0.1% 5.51 KB 5.51 KB 1.83 KB 1.83 KB NODE_DEV
react-dom-unstable-fizz.node.production.min.js 0.0% 🔺+0.3% 1.17 KB 1.17 KB 665 B 667 B NODE_PROD
react-dom-unstable-fizz.browser.development.js 0.0% +0.1% 5.26 KB 5.26 KB 1.77 KB 1.77 KB UMD_DEV
ReactDOMTesting-dev.js 0.0% 0.0% 909.54 KB 909.84 KB 203.08 KB 203.16 KB FB_WWW_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.3% 1.2 KB 1.2 KB 703 B 705 B UMD_PROD
ReactDOMTesting-prod.js 0.0% 🔺+0.1% 378.56 KB 378.71 KB 69.13 KB 69.17 KB FB_WWW_PROD
ReactDOMTesting-profiling.js 0.0% +0.1% 378.56 KB 378.71 KB 69.13 KB 69.17 KB FB_WWW_PROFILING
react-dom-unstable-fizz.browser.development.js 0.0% +0.1% 4.77 KB 4.77 KB 1.67 KB 1.67 KB NODE_DEV
react-dom-test-utils.production.min.js 0.0% 0.0% 13.12 KB 13.12 KB 4.81 KB 4.81 KB NODE_PROD
react-dom-server.node.development.js 0.0% 0.0% 153.88 KB 153.88 KB 40.75 KB 40.75 KB NODE_DEV
react-dom.development.js 0.0% 0.0% 941.62 KB 941.92 KB 206.08 KB 206.16 KB UMD_DEV
react-dom-server.browser.development.js 0.0% -0.0% 162.17 KB 162.17 KB 41.26 KB 41.26 KB UMD_DEV
react-dom.production.min.js 0.0% 0.0% 124.7 KB 124.76 KB 39.92 KB 39.94 KB UMD_PROD
ReactDOMForked-dev.js 0.0% 0.0% 1008.93 KB 1009.23 KB 224.65 KB 224.73 KB FB_WWW_DEV
react-dom-server.browser.production.min.js 0.0% -0.0% 23.4 KB 23.4 KB 8.61 KB 8.6 KB UMD_PROD
react-dom.profiling.min.js 0.0% 0.0% 128.58 KB 128.63 KB 41.07 KB 41.07 KB UMD_PROFILING
ReactDOMForked-prod.js 0.0% 🔺+0.1% 422.55 KB 422.69 KB 74.03 KB 74.07 KB FB_WWW_PROD
react-dom.development.js 0.0% 0.0% 896.48 KB 896.77 KB 203.55 KB 203.61 KB NODE_DEV
ReactDOMForked-profiling.js 0.0% 0.0% 432.89 KB 433.04 KB 75.76 KB 75.79 KB FB_WWW_PROFILING
react-dom-server.browser.development.js 0.0% 0.0% 152.66 KB 152.66 KB 40.49 KB 40.49 KB NODE_DEV
react-dom.production.min.js 0.0% 🔺+0.1% 124.89 KB 124.95 KB 39.03 KB 39.05 KB NODE_PROD

Size changes (experimental)

Generated by 🚫 dangerJS against 4ba1b2f

@trueadm trueadm changed the title Fix root issue Handle legacy root contains in clearContainer Apr 29, 2020
@trueadm trueadm reopened this Apr 29, 2020
@trueadm
Copy link
Contributor Author

trueadm commented Apr 29, 2020 •
edited
Loading

I've updated this PR with another possible fix. I'll leave this PR just to show the changes anyway, but there might be an alternative approach we might want to take instead.

@trueadm trueadm force-pushed the fix-root-issue branch 3 times, most recently from aa046af to fc25111 Compare April 29, 2020 23:21
@bvaughn
Copy link
Contributor

bvaughn commented Apr 30, 2020 •
edited
Loading

This PR fixes the regression we're seeing internally. I think there might be a better place to check for this though. Let me post a potential alternate PR here shortly...

Edit Potential alternate #18792

@bvaughn bvaughn mentioned this pull request Apr 30, 2020
Merged
@trueadm trueadm closed this Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@bvaughn bvaughn bvaughn left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@trueadm @gaearon @sizebot @bvaughn @facebook-github-bot