Add ReactDOMClient to ServerIntegration(Hooks|NewContext) #28135

rickhanlonii · 2024-01-28T00:17:15Z

Overview

Branched off #28130

Failing Fixed by @eps1lon

Most of the tests pass, but there are 3 tests that have additional warnings due to client render error retries.

For example, before we would log:

Warning: Do not call Hooks inside useEffect(...), useMemo(...), or other built-in Hooks.
Warning: Expected server HTML to contain a matching text node for "0" in <div>.

And now we log

Warning: Do not call Hooks inside useEffect(...), useMemo(...), or other built-in Hooks.
Warning: Expected server HTML to contain a matching text node for "0" in <div>.
Warning: Do not call Hooks inside useEffect(...), useMemo(...), or other built-in Hooks.

We can't just update the expected error count for these tests, because the additional error only happens on the client. So I need some guidance on how to fix these.

react-sizebot · 2024-01-28T00:24:40Z

Comparing: 9aef5d2...1365876

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.59 kB	176.59 kB	=	55.01 kB	55.01 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	178.69 kB	178.69 kB	=	55.64 kB	55.64 kB
facebook-www/ReactDOM-prod.classic.js	=	573.52 kB	573.52 kB	=	100.99 kB	100.99 kB
facebook-www/ReactDOM-prod.modern.js	=	557.29 kB	557.29 kB	=	98.07 kB	98.07 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 1365876

eps1lon · 2024-01-28T10:01:49Z

packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js

-      const domNode = await render(<App />, 1);
+      const domNode = await render(
+        <App />,
+        render === clientRenderOnBadMarkup


We use this pattern in other tests as well. E.g. packages/react-dom/src/__tests__/ReactDOMServerIntegrationElements-test.js makes extensive usage of assertions based on the renderer.

Oh snap nice

@eps1lon

## Overview Branched off #28130 ### ~Failing~ Fixed by @eps1lon Most of the tests pass, but there are 3 tests that have additional warnings due to client render error retries. For example, before we would log: ``` Warning: Do not call Hooks inside useEffect(...), useMemo(...), or other built-in Hooks. Warning: Expected server HTML to contain a matching text node for "0" in <div>. ``` And now we log ``` Warning: Do not call Hooks inside useEffect(...), useMemo(...), or other built-in Hooks. Warning: Expected server HTML to contain a matching text node for "0" in <div>. Warning: Do not call Hooks inside useEffect(...), useMemo(...), or other built-in Hooks. ``` We can't just update the expected error count for these tests, because the additional error only happens on the client. So I need some guidance on how to fix these. --------- Co-authored-by: Sebastian Silbermann <sebastian.silbermann@klarna.com>

@eps1lon

…8135) ## Overview Branched off facebook#28130 ### ~Failing~ Fixed by @eps1lon Most of the tests pass, but there are 3 tests that have additional warnings due to client render error retries. For example, before we would log: ``` Warning: Do not call Hooks inside useEffect(...), useMemo(...), or other built-in Hooks. Warning: Expected server HTML to contain a matching text node for "0" in <div>. ``` And now we log ``` Warning: Do not call Hooks inside useEffect(...), useMemo(...), or other built-in Hooks. Warning: Expected server HTML to contain a matching text node for "0" in <div>. Warning: Do not call Hooks inside useEffect(...), useMemo(...), or other built-in Hooks. ``` We can't just update the expected error count for these tests, because the additional error only happens on the client. So I need some guidance on how to fix these. --------- Co-authored-by: Sebastian Silbermann <sebastian.silbermann@klarna.com>

@eps1lon

## Overview Branched off #28130 ### ~Failing~ Fixed by @eps1lon Most of the tests pass, but there are 3 tests that have additional warnings due to client render error retries. For example, before we would log: ``` Warning: Do not call Hooks inside useEffect(...), useMemo(...), or other built-in Hooks. Warning: Expected server HTML to contain a matching text node for "0" in <div>. ``` And now we log ``` Warning: Do not call Hooks inside useEffect(...), useMemo(...), or other built-in Hooks. Warning: Expected server HTML to contain a matching text node for "0" in <div>. Warning: Do not call Hooks inside useEffect(...), useMemo(...), or other built-in Hooks. ``` We can't just update the expected error count for these tests, because the additional error only happens on the client. So I need some guidance on how to fix these. --------- Co-authored-by: Sebastian Silbermann <sebastian.silbermann@klarna.com> DiffTrain build for commit fa6674b.

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

rickhanlonii force-pushed the rh/server-test-utils-hooks branch from b30c04a to d1eb303 Compare January 28, 2024 00:20

rickhanlonii changed the title ~~[Failing] Add ReactDOMClient to ServerIntegrationHooks~~ [Failing] Add ReactDOMClient to ServerIntegration(Hooks|NewContext) Jan 28, 2024

eps1lon reviewed Jan 28, 2024

View reviewed changes

eps1lon force-pushed the rh/server-test-utils-hooks branch 2 times, most recently from 110637a to d0fada2 Compare January 28, 2024 10:21

eps1lon approved these changes Jan 28, 2024

View reviewed changes

rickhanlonii marked this pull request as ready for review January 28, 2024 21:08

rickhanlonii changed the title ~~[Failing] Add ReactDOMClient to ServerIntegration(Hooks|NewContext)~~ Add ReactDOMClient to ServerIntegration(Hooks|NewContext) Jan 28, 2024

Add ReactDOMClient to ServerIntegrationTestUtils

e204d76

rickhanlonii force-pushed the rh/server-test-utils branch from 233b65d to e204d76 Compare February 1, 2024 20:53

rickhanlonii and others added 2 commits February 1, 2024 16:05

Add ReactDOMClient to ServerIntegrationHooks

b200a8b

Handle different error counts depending on renderer

1365876

rickhanlonii force-pushed the rh/server-test-utils-hooks branch from d0fada2 to 1365876 Compare February 1, 2024 21:06

Base automatically changed from rh/server-test-utils to main February 1, 2024 23:27

rickhanlonii merged commit fa6674b into main Feb 1, 2024
36 checks passed

rickhanlonii deleted the rh/server-test-utils-hooks branch February 1, 2024 23:32

gnoff mentioned this pull request Feb 5, 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ReactDOMClient to ServerIntegration(Hooks|NewContext) #28135

Add ReactDOMClient to ServerIntegration(Hooks|NewContext) #28135

rickhanlonii commented Jan 28, 2024 •

edited

react-sizebot commented Jan 28, 2024 •

edited

eps1lon Jan 28, 2024

rickhanlonii Jan 28, 2024

Add ReactDOMClient to ServerIntegration(Hooks|NewContext) #28135

Add ReactDOMClient to ServerIntegration(Hooks|NewContext) #28135

Conversation

rickhanlonii commented Jan 28, 2024 • edited

Overview

Failing Fixed by @eps1lon

react-sizebot commented Jan 28, 2024 • edited

Critical size changes

Significant size changes

eps1lon Jan 28, 2024

Choose a reason for hiding this comment

rickhanlonii Jan 28, 2024

Choose a reason for hiding this comment

rickhanlonii commented Jan 28, 2024 •

edited

react-sizebot commented Jan 28, 2024 •

edited