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

[WIP] Diffs for hydration errors #24250

Closed
wants to merge 5 commits into from
Closed

[WIP] Diffs for hydration errors #24250

wants to merge 5 commits into from

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Apr 1, 2022 •
edited
Loading

  • Errors
    • Extra server element
    • Extra server text
    • Extra client element
    • Extra client text
    • Extra server attributes
    • Prop mismatch
  • Formatting
    • Parent tag and attributes
    • Sibling tag/text and attributes
    • Child tag/text and attributes
    • Ellipsis to indicate more siblings before or after
  • Special cases
    • Document/fragment parent
    • Long attributes
    • Too many attributes
    • Quotes in attributes
    • Long text
    • Skip over non-element/text siblings
  • Other stuff
    • Flow

This was referenced Apr 1, 2022
Closed
Closed
@facebook-github-bot facebook-github-bot added the React Core Team Opened by a member of the React Core Team label Apr 1, 2022
@sizebot
Copy link

sizebot commented Apr 1, 2022 •
edited
Loading

Comparing: 0dc4e66...35c1424

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 = 131.52 kB 131.52 kB = 42.10 kB 42.10 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 136.79 kB 136.79 kB = 43.68 kB 43.68 kB
facebook-www/ReactDOM-prod.classic.js = 441.02 kB 441.02 kB = 80.44 kB 80.44 kB
facebook-www/ReactDOM-prod.modern.js = 426.28 kB 426.28 kB = 78.25 kB 78.25 kB
facebook-www/ReactDOMForked-prod.classic.js = 441.02 kB 441.02 kB = 80.44 kB 80.44 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
facebook-www/ReactDOMTesting-dev.modern.js +0.53% 1,033.54 kB 1,039.06 kB +0.45% 232.82 kB 233.87 kB
oss-stable-semver/react-dom/umd/react-dom.development.js +0.52% 1,072.44 kB 1,078.02 kB +0.44% 232.01 kB 233.02 kB
oss-stable/react-dom/umd/react-dom.development.js +0.52% 1,072.44 kB 1,078.02 kB +0.44% 232.01 kB 233.02 kB
facebook-www/ReactDOMTesting-dev.classic.js +0.52% 1,062.15 kB 1,067.67 kB +0.44% 238.49 kB 239.55 kB
oss-stable-semver/react-dom/cjs/react-dom.development.js +0.51% 1,022.14 kB 1,027.40 kB +0.44% 229.47 kB 230.49 kB
oss-stable/react-dom/cjs/react-dom.development.js +0.51% 1,022.14 kB 1,027.40 kB +0.44% 229.47 kB 230.49 kB
oss-experimental/react-dom/cjs/react-dom-unstable_testing.development.js +0.51% 1,034.13 kB 1,039.39 kB +0.44% 233.26 kB 234.29 kB
facebook-www/ReactDOMForked-dev.modern.js +0.51% 1,127.64 kB 1,133.36 kB +0.43% 249.63 kB 250.70 kB
oss-experimental/react-dom/umd/react-dom.development.js +0.51% 1,102.97 kB 1,108.55 kB +0.44% 238.26 kB 239.30 kB
oss-experimental/react-dom/cjs/react-dom.development.js +0.50% 1,051.26 kB 1,056.52 kB +0.43% 235.69 kB 236.72 kB
facebook-www/ReactDOMForked-dev.classic.js +0.50% 1,151.61 kB 1,157.33 kB +0.43% 253.99 kB 255.08 kB
facebook-www/ReactDOM-dev.modern.js +0.49% 1,127.64 kB 1,133.16 kB +0.42% 249.64 kB 250.68 kB
facebook-www/ReactDOM-dev.classic.js +0.48% 1,151.61 kB 1,157.12 kB +0.42% 253.99 kB 255.07 kB

Generated by 🚫 dangerJS against 35c1424

sompylasar
sompylasar reviewed Apr 2, 2022
View reviewed changes
packages/react-dom/src/client/ReactDOMComponent.js
'Did not expect server HTML to contain a <%s> in <%s>.',
child.nodeName.toLowerCase(),
parentNode.nodeName.toLowerCase(),
'The content rendered by the server and the client did not match ' +
Copy link
Contributor

Choose a reason for hiding this comment

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

The explanation in detail sounds great, but I feel there are too many words that are the same across multiple messages until the very essence of this message which is "the server has rendered an extra text node". Could be easier and quicker to grasp the gist if the varying part of the message was at the beginning, and the repetitive explanation at the end.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah good point. I'll take another pass to clean them up after all of them are covered.

@balazsorban44 balazsorban44 mentioned this pull request Aug 9, 2022
Closed
11 tasks
@gnoff gnoff self-assigned this Nov 28, 2022
@karlhorky
Copy link
Contributor

karlhorky commented Nov 28, 2023 •
edited
Loading

This would be great, also for Next.js:

@karlhorky karlhorky mentioned this pull request Nov 28, 2023
Closed
@himself65
Copy link
Contributor

Any update for this PR?

@gaearon
Copy link
Collaborator Author

gaearon commented Dec 4, 2023

No, I'm not actively working on this so there won't be any update.

@gaearon
Copy link
Collaborator Author

gaearon commented Dec 4, 2023

Someone motivated could pick this up though!

@himself65
Copy link
Contributor

Someone motivated could pick this up though!

I will try to continue when I'm avaliable!

@gaearon
Copy link
Collaborator Author

gaearon commented Dec 4, 2023

That would be great! The first step would probably be to get this rebased and/or rewrite it based on the tests I added and the logic I've included. Then you can look at the remaining TODOs.

@himself65 himself65 mentioned this pull request Dec 7, 2023
Closed
21 tasks
himself65
himself65 reviewed Jan 3, 2024
View reviewed changes
packages/react-dom/src/client/ReactDOMComponent.js

function formatNode(node, indentation) {
switch (node.nodeType) {
// TODO: use constants
Copy link
Contributor

Choose a reason for hiding this comment

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

What does use constants mean here? Isn't here already constants?

@sebmarkbage
Copy link
Collaborator

Going with a more invasive refactor and different approach in #28512.

@sebmarkbage sebmarkbage closed this Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@himself65 himself65 himself65 left review comments

@sompylasar sompylasar sompylasar left review comments

Assignees

@gnoff gnoff

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.
8 participants
@gaearon @sizebot @karlhorky @himself65 @sebmarkbage @sompylasar @gnoff @facebook-github-bot