Skip to content

Conversation

@gnoff
Copy link
Collaborator

@gnoff gnoff commented Feb 8, 2023

as reported in #26128 ReactDOM.render(..., document) crashed when enableHostSingletons was on. This is because it had a different way of clearing the container than createRoot(document). I updated the legacy implementation to share the clearing behavior of creatRoot which will preserve the singleton instances.

I also removed the warning saying not to use document.body as a container

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Feb 8, 2023
@gnoff gnoff requested review from eps1lon and sebmarkbage February 8, 2023 19:06
@react-sizebot
Copy link

react-sizebot commented Feb 8, 2023

Comparing: 758fc7f...3de6076

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 = 154.84 kB 154.82 kB = 49.12 kB 49.11 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 156.85 kB 156.82 kB = 49.78 kB 49.72 kB
facebook-www/ReactDOM-prod.classic.js = 533.79 kB 533.52 kB = 95.06 kB 94.98 kB
facebook-www/ReactDOM-prod.modern.js = 518.81 kB 518.79 kB = 92.82 kB 92.83 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 3de6076

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

Wow. People still use this api with the warning?

Copy link
Collaborator

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Still seeing this diff:

()"` |
-| `version=(numeric string)`| (changed, ssr mismatch)| `"42"` |    
-| `version=(-1)`| (changed, ssr mismatch)| `"-1"` |
-| `version=(0)`| (changed, ssr mismatch)| `"0"` |-| `version=(integer)`| (changed, ssr mismatch)| `"1"` |
-| `version=(NaN)`| (changed, warning, ssr mismatch)| `"NaN"` |     -| `version=(float)`| (changed, ssr mismatch)| `"99.99"` |
-| `version=(true)`| (initial, warning)| `<empty string>` |-| `version=(false)`| (initial, warning)| `<empty string>` |        
-| `version=(string 'true')`| (changed, ssr mismatch)| `"true"` |   -| `version=(string 'false')`| (changed, ssr mismatch)| `"false"` | 
-| `version=(string 'on')`| (changed, ssr mismatch)| `"on"` |       
-| `version=(string 'off')`| (changed, ssr mismatch)| `"off"` |     
-| `version=(symbol)`| (initial, warning)| `<empty string>` |       
-| `version=(function)`| (initial, warning)| `<empty string>` |     
-| `version=(null)`| (initial)| `<empty string>` |
-| `version=(undefined)`| (initial)| `<empty string>` |
+| `version=(string)`| (initial, ssr mismatch)| `<undefined>` |     
+| `version=(empty string)`| (initial, ssr mismatch)| `<undefined>` |
+| `version=(array with string)`| (initial, ssr mismatch)| `<undefined>` |
+| `version=(empty array)`| (initial, ssr mismatch)| `<undefined>` |
+| `version=(object)`| (initial, ssr mismatch)| `<undefined>` |     
+| `version=(numeric string)`| (initial, ssr mismatch)| `<undefined>` |
+| `version=(-1)`| (initial, ssr mismatch)| `<undefined>` |
+| `version=(0)`| (initial, ssr mismatch)| `<undefined>` |
+| `version=(integer)`| (initial, ssr mismatch)| `<undefined>` |    
+| `version=(NaN)`| (initial, warning, ssr mismatch)| `<undefined>` |
+| `version=(float)`| (initial, ssr mismatch)| `<undefined>` |      +| `version=(true)`| (initial, warning, ssr mismatch)| `<undefined>` |
+| `version=(false)`| (initial, warning, ssr mismatch)| `<undefined>` |
+| `version=(string 'true')`| (initial, ssr mismatch)| `<undefined>` |
+| `version=(string 'false')`| (initial, ssr mismatch)| `<undefined>` |
+| `version=(string 'on')`| (initial, ssr mismatch)| `<undefined>` |
+| `version=(string 'off')`| (initial, ssr mismatch)| `<undefined>` |+| `version=(symbol)`| (initial, warning, ssr mismatch)| `<undefined>` |
+| `version=(function)`| (initial, warning, ssr mismatch)| `<undefined>` |
+| `version=(null)`| (initial, ssr mismatch)| `<undefined>` |       
+| `version=(undefined)`| (initial, ssr mismatch)| `<undefined>` |  

 ## `version` (on `<svg>` inside `<div>`)
 | Test Case | Flags | Result |

which makes it seem like no value is rendered. Will just replace this test with one using lang since version is obsolete anyway.

@gnoff gnoff merged commit a3152ed into facebook:main Feb 8, 2023
@gnoff gnoff deleted the bugfix-singletons-crash branch February 8, 2023 19:32
github-actions bot pushed a commit that referenced this pull request Feb 8, 2023
as reported in #26128 `ReactDOM.render(..., document)` crashed when
`enableHostSingletons` was on. This is because it had a different way of
clearing the container than `createRoot(document)`. I updated the legacy
implementation to share the clearing behavior of `creatRoot` which will
preserve the singleton instances.

I also removed the warning saying not to use `document.body` as a
container

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

5 participants