Convert refs-destruction to createRoot #28011

rickhanlonii · 2024-01-19T19:33:04Z

No description provided.

rickhanlonii · 2024-01-19T19:34:24Z

packages/react-dom/src/__tests__/refs-destruction-test.js

        document.body.appendChild(this.div);
        this.componentDidUpdate();
      }

      componentDidUpdate() {
-        ReactDOM.render(<div>{this.props.children}</div>, this.div);
+        setTimeout(() => {


This is clowning af but the test before was clowny af (rendering and unmounting roots in lifecycles) so I don't want to rewrite it and risk losing whatever it was testing for.

Needs to be flushSync because you can't use async/await in lifecycles. Needs to be a timeout because we warn if you use flushSync in lifecycles. The act flush ensures the timeout is fired.

I don't really understand what's being tested here. We could also mark it as legacy and gate the tests.

It's testing that if you have a nested root that you animate on unmount, there's no error. This is how you did portals before createPortal, so I don't think it's legacy until we remove class components.

@kassens but I'm curious what you mean by gating, if this was legacy what gate would we use?

react-sizebot · 2024-01-19T19:36:53Z

Comparing: 64d0c94...d3567c5

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	=	176.24 kB	176.24 kB	=	54.88 kB	54.88 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	178.34 kB	178.34 kB	=	55.53 kB	55.53 kB
facebook-www/ReactDOM-prod.classic.js	=	571.94 kB	571.94 kB	=	100.56 kB	100.56 kB
facebook-www/ReactDOM-prod.modern.js	=	555.70 kB	555.70 kB	=	97.62 kB	97.62 kB
test_utils/ReactAllWarnings.js	Deleted	67.55 kB	0.00 kB	Deleted	16.54 kB	0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
test_utils/ReactAllWarnings.js	Deleted	67.55 kB	0.00 kB	Deleted	16.54 kB	0.00 kB

Generated by 🚫 dangerJS against d3567c5

kassens · 2024-01-24T15:42:20Z

packages/react-dom/src/__tests__/refs-destruction-test.js


-    ReactDOM.unmountComponentAtNode(container);
+    root.unmount();


I'm surprised this works, I thought unmount is auto-batched like render.

It's not, we wrap unmount in flushSync. If there was a render before unmount that hasn't flushed yet, then it would be batched with the sync flush for unmount:

react/packages/react-dom/src/client/ReactDOMRoot.js

Line 155 in f498aa2

flushSync(() => {

kassens · 2024-01-24T15:45:42Z

packages/react-dom/src/__tests__/refs-destruction-test.js

        document.body.appendChild(this.div);
        this.componentDidUpdate();
      }

      componentDidUpdate() {
-        ReactDOM.render(<div>{this.props.children}</div>, this.div);
+        setTimeout(() => {


I don't really understand what's being tested here. We could also mark it as legacy and gate the tests.

DiffTrain build for commit 72411c4.

Convert refs-destruction to createRoot

d3567c5

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jan 19, 2024

rickhanlonii commented Jan 19, 2024

View reviewed changes

kassens approved these changes Jan 24, 2024

View reviewed changes

rickhanlonii merged commit 72411c4 into facebook:main Jan 24, 2024
36 checks passed

rickhanlonii deleted the rh/tests-ref-destruction branch January 24, 2024 19:30

gnoff mentioned this pull request Feb 1, 2024

Update React from 60a927d04 to 2bc7d336a vercel/next.js#61522

Merged

ztanner mentioned this pull request Mar 19, 2024

(backport) Update React from 60a927d04 to 14898b6a9 vercel/next.js#63442

Closed

EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024

Convert refs-destruction to createRoot (facebook#28011)

0a71c7c

bigfootjon pushed a commit that referenced this pull request Apr 18, 2024

Convert refs-destruction to createRoot (#28011)

e43ef93

DiffTrain build for commit 72411c4.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert refs-destruction to createRoot #28011

Convert refs-destruction to createRoot #28011

rickhanlonii commented Jan 19, 2024

rickhanlonii Jan 19, 2024

rickhanlonii Jan 19, 2024

kassens Jan 24, 2024

rickhanlonii Jan 24, 2024

rickhanlonii Jan 24, 2024

react-sizebot commented Jan 19, 2024

kassens Jan 24, 2024

rickhanlonii Jan 24, 2024

kassens Jan 24, 2024

Convert refs-destruction to createRoot #28011

Convert refs-destruction to createRoot #28011

Conversation

rickhanlonii commented Jan 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

react-sizebot commented Jan 19, 2024

Critical size changes

Significant size changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment