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

[Fiber] retain scripts on clearContainer and clearSingleton #26871

Merged
merged 1 commit into from
May 30, 2023

Conversation

gnoff
Copy link
Collaborator

@gnoff gnoff commented May 30, 2023

clearContainer and clearSingleton both assumed scripts could be safely removed from the DOM because normally once a script has been inserted into the DOM it is executable and removing it, even synchronously, will not prevent it from running. However There is an edge case in a couple browsers (Chrome at least) where during HTML streaming if a script is opened and not yet closed the script will be inserted into the document but not yet executed. If the script is removed from the document before the end tag is parsed then the script will not run. This change causes clearContainer and clearSingleton to retain script elements. This is generally thought to be safe because if we are calling these methods we are no longer hydrating the container or the singleton and the scripts execution will happen regardless.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels May 30, 2023
@gnoff gnoff requested review from sebmarkbage and acdlite May 30, 2023 17:44
@react-sizebot
Copy link

react-sizebot commented May 30, 2023

Comparing: a1f9758...6a0e253

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js +0.02% 164.23 kB 164.25 kB +0.03% 51.77 kB 51.78 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.02% 171.67 kB 171.69 kB +0.03% 54.01 kB 54.02 kB
facebook-www/ReactDOM-prod.classic.js +0.01% 570.42 kB 570.48 kB +0.02% 100.64 kB 100.66 kB
facebook-www/ReactDOM-prod.modern.js +0.01% 554.16 kB 554.22 kB +0.02% 97.82 kB 97.84 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 6a0e253

…y removed from the DOM because normally once a script has been inserted into the DOM it is executable and removing it, even synchronously, will not prevent it from running. However There is an edge case in a couple browsers (Chrome at least) where during HTML streaming if a script is opened and not yet closed the script will be inserted into the document but not yet executed. If the script is removed from the document before the end tag is parsed then the script will not run. This change causes clearContainer and clearSingleton to retain script elements. This is generally thought to be safe because if we are calling these methods we are no longer hydrating the container or the singleton and the scripts execution will happen regardless.
@gnoff gnoff force-pushed the retain-scripts-on-clear branch from 6517bcd to 6a0e253 Compare May 30, 2023 20:00
@gnoff gnoff merged commit 1cea384 into facebook:main May 30, 2023
@gnoff gnoff deleted the retain-scripts-on-clear branch May 30, 2023 20:12
github-actions bot pushed a commit that referenced this pull request May 30, 2023
clearContainer and clearSingleton both assumed scripts could be safely
removed from the DOM because normally once a script has been inserted
into the DOM it is executable and removing it, even synchronously, will
not prevent it from running. However There is an edge case in a couple
browsers (Chrome at least) where during HTML streaming if a script is
opened and not yet closed the script will be inserted into the document
but not yet executed. If the script is removed from the document before
the end tag is parsed then the script will not run. This change causes
clearContainer and clearSingleton to retain script elements. This is
generally thought to be safe because if we are calling these methods we
are no longer hydrating the container or the singleton and the scripts
execution will happen regardless.

DiffTrain build for [1cea384](1cea384)
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…book#26871)

clearContainer and clearSingleton both assumed scripts could be safely
removed from the DOM because normally once a script has been inserted
into the DOM it is executable and removing it, even synchronously, will
not prevent it from running. However There is an edge case in a couple
browsers (Chrome at least) where during HTML streaming if a script is
opened and not yet closed the script will be inserted into the document
but not yet executed. If the script is removed from the document before
the end tag is parsed then the script will not run. This change causes
clearContainer and clearSingleton to retain script elements. This is
generally thought to be safe because if we are calling these methods we
are no longer hydrating the container or the singleton and the scripts
execution will happen regardless.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
clearContainer and clearSingleton both assumed scripts could be safely
removed from the DOM because normally once a script has been inserted
into the DOM it is executable and removing it, even synchronously, will
not prevent it from running. However There is an edge case in a couple
browsers (Chrome at least) where during HTML streaming if a script is
opened and not yet closed the script will be inserted into the document
but not yet executed. If the script is removed from the document before
the end tag is parsed then the script will not run. This change causes
clearContainer and clearSingleton to retain script elements. This is
generally thought to be safe because if we are calling these methods we
are no longer hydrating the container or the singleton and the scripts
execution will happen regardless.

DiffTrain build for commit 1cea384.
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.

4 participants