Add Diffs to Hydration Warnings #28512
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: f7aa5e0...2f2eb4d Critical size changesIncludes critical production bundles, as well as any change greater than 2%: Significant size changesIncludes any change greater than 0.2%: Expand to show
|
sebmarkbage
force-pushed
the
diffhydration
branch
from
c09fe9e
to
f3704ea
Compare
| <div className="parent"> | ||
| <main className="child"> | ||
| + client | ||
| - server |
There was a problem hiding this comment.
Is there a reason + added lines are printed before - removed lines? Usually in diff visualizations, removed lines are printed first.
There was a problem hiding this comment.
@sompylasar nice to bump into you here haha, it's been a while since you were adding this...
There was a problem hiding this comment.
There’s an awkward scenario that happens at the end where if we have more client siblings we don’t necessarily know if they would’ve matched so we cut off next siblings with “…”. However if there are extra server nodes at the tail then we do list them as removed next siblings which puts them at the end. Sometimes a swap looks like this scenario so I just followed that ordering to avoid sometimes flipping it.
I don’t think it’s necessarily inherent though. Could probably be changed.
| '{"text-decoration":"none","color":"black","height":"10px"}' + | ||
| ' Client: ' + | ||
| '{"textDecoration":"none","color":"black","height":"10px"}', // note that this is no difference | ||
| "A tree hydrated but some attributes of the server rendered HTML didn't match the client properties.", |
There was a problem hiding this comment.
maybe drop "rendered"? easy to read it as a verb and then the sentence structure breaks
|
I'm sure I could just read the tests but — what's the PROD situation? Do you get a diff in PROD or is it less detailed? |
|
You get nothing in prod. Just minimized error with the error code linking to a page with this message but no context. However, if you're logging it yourself you do have access to component stacks in onRecoverableError but it's up to a production logging tool to format that and it won't have access to props etc. |
| + style={{opacity:1}} | ||
| - style={{opacity:"0"}} |
There was a problem hiding this comment.
Does this imply to users that part of the problem is one is a string and the other is a number? I wonder if we should try to parse the html attribute as a number if the hydration prop is also a number. It'd have to be clever like
if (typeof prop === 'number' && parseInt(attr).toString() === attr) {
return parseInt(attr)
} else {
return attr
}
There was a problem hiding this comment.
unrelated, but maybe worth adding a space for readability? opacity: 1
There was a problem hiding this comment.
Yes but I think you'll find that this is not the only place we could be clever. The more important one might be to find equivalences of values. E.g. if something else differs, we'll generate a diff and the diff will itself think that both 0 and "0" is different and it'll show up as a diff. Most of the time it's not as easy that though because most of the time you can't have a unitless number. So for example "1px" is equivalent to 1. It'll also ignore any style prop that isn't parsed since we read back the styles using the browser. So a browser specific one might show up as diff if something else in the stylesheet differences.
I wouldn't over rotate on the unit tests since they're just the ones that used to be here and all the other ones don't use snapshots that test this. There's more interesting cases to be found in production code.
I ended up not including spaces for these cases because the goal is to keep it compact enough to fit on one line. Especially in the case where these are formatted on contextual nodes because I always enforce it to take up at most one line so a few extra spaces can mean that a prop gets dropped instead. Don't really want to do a constraint solving where it tries to fit it.
| + style={{opacity:1}} | ||
| - style={{opacity:"0"}} |
There was a problem hiding this comment.
unrelated, but maybe worth adding a space for readability? opacity: 1
sebmarkbage
force-pushed
the
diffhydration
branch
from
f3704ea
to
69263c6
Compare
This assumes that the description returns the string "Suspense" and that any other hydratable node won't do that.
This collapses parent nodes of larger trees unless they're a fork, has diff information or is close to a leaf of the tree that has diff info.
Otherwise we won't have diffs of parent props when a child later throws.
sebmarkbage
force-pushed
the
diffhydration
branch
from
69263c6
to
2f2eb4d
Compare
Stacked on #28502. This builds on the mechanism in #28502 by adding a diff of everything we've collected so far to the thrown error or logged error. This isn't actually a longest common subsequence diff. This means that there are certain cases that can appear confusing such as a node being added/removed when it really would've appeared later in the list. In fact once a node mismatches, we abort rendering so we don't have the context of what would've been rendered. It's not quite right to use the result of the recovery render because it can use client-only code paths using useSyncExternalStore which would yield false differences. That's why diffing the HTML isn't quite right. I also present abstract components in the stack, these are presented with the client props and no diff since we don't have the props that were on the server. The lack of difference might be confusing but it's useful for context. The main thing that's data new here is that we're adding some siblings and props for context. Examples in the [snapshot commit](e14532f). DiffTrain build for [2ec2aae](2ec2aae)
Stacked on facebook#28502. This builds on the mechanism in facebook#28502 by adding a diff of everything we've collected so far to the thrown error or logged error. This isn't actually a longest common subsequence diff. This means that there are certain cases that can appear confusing such as a node being added/removed when it really would've appeared later in the list. In fact once a node mismatches, we abort rendering so we don't have the context of what would've been rendered. It's not quite right to use the result of the recovery render because it can use client-only code paths using useSyncExternalStore which would yield false differences. That's why diffing the HTML isn't quite right. I also present abstract components in the stack, these are presented with the client props and no diff since we don't have the props that were on the server. The lack of difference might be confusing but it's useful for context. The main thing that's data new here is that we're adding some siblings and props for context. Examples in the [snapshot commit](facebook@e14532f).
Stacked on #28502. This builds on the mechanism in #28502 by adding a diff of everything we've collected so far to the thrown error or logged error. This isn't actually a longest common subsequence diff. This means that there are certain cases that can appear confusing such as a node being added/removed when it really would've appeared later in the list. In fact once a node mismatches, we abort rendering so we don't have the context of what would've been rendered. It's not quite right to use the result of the recovery render because it can use client-only code paths using useSyncExternalStore which would yield false differences. That's why diffing the HTML isn't quite right. I also present abstract components in the stack, these are presented with the client props and no diff since we don't have the props that were on the server. The lack of difference might be confusing but it's useful for context. The main thing that's data new here is that we're adding some siblings and props for context. Examples in the [snapshot commit](e14532f). DiffTrain build for commit 2ec2aae.
Stacked on #28502.
This builds on the mechanism in #28502 by adding a diff of everything we've collected so far to the thrown error or logged error.
This isn't actually a longest common subsequence diff. This means that there are certain cases that can appear confusing such as a node being added/removed when it really would've appeared later in the list. In fact once a node mismatches, we abort rendering so we don't have the context of what would've been rendered. It's not quite right to use the result of the recovery render because it can use client-only code paths using useSyncExternalStore which would yield false differences. That's why diffing the HTML isn't quite right.
I also present abstract components in the stack, these are presented with the client props and no diff since we don't have the props that were on the server. The lack of difference might be confusing but it's useful for context.
The main thing that's data new here is that we're adding some siblings and props for context.
Examples in the snapshot commit.