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.

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

Add Promise as a child test to Flight fixture #28778

Merged
merged 3 commits into from
Apr 8, 2024
Merged

Add Promise as a child test to Flight fixture #28778

merged 3 commits into from
Apr 8, 2024

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Apr 8, 2024 •
edited
Loading

Adds a test for promise as a child that was fixed by #28284. Since the fix only included tests for React.Children I also added another regression test based on the original bug report in #27247.

Test plan

I can't repro this in our unit test suite so I use the Flight fixture which fails on any console error.

  • Reverting the fix fails CI: https://app.circleci.com/pipelines/github/facebook/react/51990/workflows/d89bc4f1-0ae0-4694-a7ed-a922ec4a7c66/jobs/828188 
     Error: expect(received).toEqual(expected) // deep equality

 - Expected  -  1
 + Received  + 38

 - Array []
 + Array [
 +   "Error: Hydration failed because the server rendered HTML didn't match the client. As a result this tree will be regenerated on the client. This can happen if a SSR-ed Client Component used:
 +
 + - A server/client branch `if (typeof window !== 'undefined')`.
 + - Variable input such as `Date.now()` or `Math.random()` which changes each time it's called.
 + - Date formatting in a user's locale which doesn't match the server.
 + - External changing data without sending a snapshot of it along with the HTML.
 + - Invalid HTML tag nesting.
 +
 + It can also happen if the client has a browser extension installed which messes with the HTML before React loaded.
 +
 + https://react.dev/link/hydration-mismatch
 +
 +   <Shell data={[...]}>
 +     <link>
 +     <App>
 +       <html lang=\"en\">
 +         <head>
 +         <body>
 +           <Container>
 +             <div>
 +               <h1>
 +               <Suspense fallback={null}>
 + +               <div data-testid=\"promise-as-a-child-test\">
 + -               Promise as a child hydrates without errors: 
 +               ...
 +
 +     at throwOnHydrationMismatch (http://localhost:3000/static/js/bundle.js:20903:19)
 +     at tryToClaimNextHydratableInstance (http://localhost:3000/static/js/bundle.js:20948:9)
 +     at updateHostComponent$1 (http://localhost:3000/static/js/bundle.js:30288:9)
 +     at beginWork (http://localhost:3000/static/js/bundle.js:31858:18)
 +     at replaySuspendedUnitOfWork (http://localhost:3000/static/js/bundle.js:38668:20)
 +     at renderRootConcurrent (http://localhost:3000/static/js/bundle.js:38366:21)
 +     at performConcurrentWorkOnRoot (http://localhost:3000/static/js/bundle.js:37342:42)
 +     at workLoop (http://localhost:3000/static/js/bundle.js:49707:38)
 +     at flushWork (http://localhost:3000/static/js/bundle.js:49677:18)
 +     at MessagePort.performWorkUntilDeadline (http://localhost:3000/static/js/bundle.js:49975:25)",
 + ]

   19 |
   20 |   await expect(consoleErrors).toEqual([]);
 > 21 |   await expect(pageErrors).toEqual([]);

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 8, 2024
@react-sizebot
Copy link

react-sizebot commented Apr 8, 2024 •
edited
Loading

Comparing: f86afca...4b32fda

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 = 168.90 kB 168.90 kB = 52.69 kB 52.69 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 170.71 kB 170.71 kB = 53.24 kB 53.24 kB
facebook-www/ReactDOM-prod.classic.js = 590.23 kB 590.23 kB = 103.62 kB 103.62 kB
facebook-www/ReactDOM-prod.modern.js = 566.56 kB 566.56 kB = 99.57 kB 99.57 kB
test_utils/ReactAllWarnings.js Deleted 64.04 kB 0.00 kB Deleted 16.02 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 64.04 kB 0.00 kB Deleted 16.02 kB 0.00 kB

Generated by 🚫 dangerJS against 4b32fda

@eps1lon eps1lon force-pushed the test/promise-as-a-child branch from f877090 to 74b1848 Compare April 8, 2024 11:13
@eps1lon eps1lon changed the title Add Promise as a child to Flight fixture Add Promise as a child test to Flight fixture Apr 8, 2024
@eps1lon eps1lon marked this pull request as ready for review April 8, 2024 11:25
@eps1lon eps1lon requested a review from sebmarkbage April 8, 2024 11:25
@sebmarkbage
Copy link
Collaborator

Wait how does that PR fix it? That was intended more as a performance optimization.

@eps1lon
Copy link
Collaborator Author

eps1lon commented Apr 8, 2024

I didn't dig further yet. But reverting the PR does produce the hydration error. Maybe it was https://github.com/facebook/react/pull/28284/files#r1483763696 specifically that fixed it?

sebmarkbage
sebmarkbage approved these changes Apr 8, 2024
View reviewed changes
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.

There seems to be something off about that logic if that's necessary. Maybe there's other cases it's broken too.

@eps1lon eps1lon merged commit e63918d into facebook:main Apr 8, 2024
38 checks passed
@eps1lon eps1lon deleted the test/promise-as-a-child branch April 8, 2024 15:06
rickhanlonii pushed a commit that referenced this pull request Apr 11, 2024
@eps1lon @rickhanlonii
Add Promise as a child test to Flight fixture (#28778)
0aae54c
rickhanlonii pushed a commit that referenced this pull request Apr 11, 2024
@eps1lon @rickhanlonii
Add Promise as a child test to Flight fixture (#28778)
96c8e74
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
@eps1lon @AndyPengc12
Add Promise as a child test to Flight fixture (facebook#28778)
5e32c42
This was referenced Apr 24, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebmarkbage sebmarkbage sebmarkbage approved these changes

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.
4 participants
@eps1lon @react-sizebot @sebmarkbage @facebook-github-bot