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

Don't mute hydration errors forcing client render #24276

Merged
merged 2 commits into from Apr 5, 2022
Merged

Don't mute hydration errors forcing client render #24276

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ca0cdce
Don't mute hydration errors forcing client render
gaearon Apr 5, 2022
380dc5b
Nits
gaearon Apr 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 14 additions & 7 deletions packages/react-dom/src/__tests__/ReactDOMFizzSuppressHydrationWarning-test.js
View file
Open in desktop
Expand Up @@ -248,9 +248,10 @@ describe('ReactDOMFizzServerHydrationWarning', () => {
]);
}).toErrorDev(
[
'Expected server HTML to contain a matching <span> in <span>',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These were being covered up, making mismatches with prod behavior hard to debug and fix.

'An error occurred during hydration. The server HTML was replaced with client content in <div>.',
Copy link
Collaborator

Choose a reason for hiding this comment

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

You still have these though. Isn't it better to have the context here for the ones we check in prod anyway? Is it worth having both? Is it confusing that there are two with subtly different timing?

Maybe it would make more sense to have the DEV only error if it was part of a larger diff across multiple mismatches which also included attributes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In other words, should we just move the insert/deletion error messages to the onRecoverableError? Perhaps batching them up if there are multiple. Oops, now you just got nerd sniped into making a larger diff view.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have another PR in the works for the diff. It's not coalesced though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But in either case I think we should fix up the inconsistency first. I.e. your concerns don't seem related to this PR per se, it just aligns the behavior for suppressHydration ones with the rest.

],
{withoutStack: true},
{withoutStack: 1},
);
} else {
// This used to not warn.
Expand Down Expand Up @@ -336,9 +337,10 @@ describe('ReactDOMFizzServerHydrationWarning', () => {
]);
}).toErrorDev(
[
'Did not expect server HTML to contain the text node "Server" in <span>',
'An error occurred during hydration. The server HTML was replaced with client content in <div>.',
],
{withoutStack: true},
{withoutStack: 1},
);
} else {
// This used to not warn.
Expand Down Expand Up @@ -389,9 +391,10 @@ describe('ReactDOMFizzServerHydrationWarning', () => {
]);
}).toErrorDev(
[
'Expected server HTML to contain a matching text node for "Client" in <span>.',
'An error occurred during hydration. The server HTML was replaced with client content in <div>.',
],
{withoutStack: true},
{withoutStack: 1},
);
} else {
// This used to not warn.
Expand Down Expand Up @@ -445,9 +448,10 @@ describe('ReactDOMFizzServerHydrationWarning', () => {
]);
}).toErrorDev(
[
'Did not expect server HTML to contain the text node "Server" in <span>.',
'An error occurred during hydration. The server HTML was replaced with client content in <div>.',
],
{withoutStack: true},
{withoutStack: 1},
);
} else {
// This used to not warn.
Expand Down Expand Up @@ -500,9 +504,10 @@ describe('ReactDOMFizzServerHydrationWarning', () => {
]);
}).toErrorDev(
[
'Expected server HTML to contain a matching text node for "Client" in <span>.',
'An error occurred during hydration. The server HTML was replaced with client content in <div>.',
],
{withoutStack: true},
{withoutStack: 1},
);
} else {
// This used to not warn.
Expand Down Expand Up @@ -630,9 +635,10 @@ describe('ReactDOMFizzServerHydrationWarning', () => {
]);
}).toErrorDev(
[
'Expected server HTML to contain a matching <p> in <div>.',
'An error occurred during hydration. The server HTML was replaced with client content in <div>.',
],
{withoutStack: true},
{withoutStack: 1},
);
} else {
// This used to not warn.
Expand Down Expand Up @@ -681,9 +687,10 @@ describe('ReactDOMFizzServerHydrationWarning', () => {
]);
}).toErrorDev(
[
'Did not expect server HTML to contain a <p> in <div>.',
'An error occurred during hydration. The server HTML was replaced with client content in <div>.',
],
{withoutStack: true},
{withoutStack: 1},
);
} else {
// This used to not warn.
Expand Down
43 changes: 31 additions & 12 deletions packages/react-dom/src/client/ReactDOMHostConfig.js
View file
Open in desktop
Expand Up @@ -62,6 +62,7 @@ import dangerousStyleValue from '../shared/dangerousStyleValue';
import {retryIfBlockedOn} from '../events/ReactDOMEventReplaying';

import {
enableClientRenderFallbackOnHydrationMismatch,
enableSuspenseServerRenderer,
enableCreateEventHandleAPI,
enableScopeAPI,
Expand Down Expand Up @@ -1004,14 +1005,20 @@ export function didNotHydrateInstance(
parentProps: Props,
parentInstance: Instance,
instance: HydratableInstance,
isConcurrentMode: boolean,
) {
if (__DEV__ && parentProps[SUPPRESS_HYDRATION_WARNING] !== true) {
if (instance.nodeType === ELEMENT_NODE) {
warnForDeletedHydratableElement(parentInstance, (instance: any));
} else if (instance.nodeType === COMMENT_NODE) {
// TODO: warnForDeletedHydratableSuspenseBoundary
} else {
warnForDeletedHydratableText(parentInstance, (instance: any));
if (__DEV__) {
if (
(enableClientRenderFallbackOnHydrationMismatch && isConcurrentMode) ||
parentProps[SUPPRESS_HYDRATION_WARNING] !== true
) {
if (instance.nodeType === ELEMENT_NODE) {
warnForDeletedHydratableElement(parentInstance, (instance: any));
} else if (instance.nodeType === COMMENT_NODE) {
// TODO: warnForDeletedHydratableSuspenseBoundary
} else {
warnForDeletedHydratableText(parentInstance, (instance: any));
}
}
}
}
Expand Down Expand Up @@ -1082,9 +1089,15 @@ export function didNotFindHydratableInstance(
parentInstance: Instance,
type: string,
props: Props,
isConcurrentMode: boolean,
) {
if (__DEV__ && parentProps[SUPPRESS_HYDRATION_WARNING] !== true) {
warnForInsertedHydratedElement(parentInstance, type, props);
if (__DEV__) {
if (
(enableClientRenderFallbackOnHydrationMismatch && isConcurrentMode) ||
parentProps[SUPPRESS_HYDRATION_WARNING] !== true
) {
warnForInsertedHydratedElement(parentInstance, type, props);
}
}
}

Expand All @@ -1093,9 +1106,15 @@ export function didNotFindHydratableTextInstance(
parentProps: Props,
parentInstance: Instance,
text: string,
isConcurrentMode: boolean,
) {
if (__DEV__ && parentProps[SUPPRESS_HYDRATION_WARNING] !== true) {
warnForInsertedHydratedText(parentInstance, text);
if (__DEV__) {
if (
(enableClientRenderFallbackOnHydrationMismatch && isConcurrentMode) ||
parentProps[SUPPRESS_HYDRATION_WARNING] !== true
) {
warnForInsertedHydratedText(parentInstance, text);
}
}
}

Expand All @@ -1104,7 +1123,7 @@ export function didNotFindHydratableSuspenseInstance(
parentProps: Props,
parentInstance: Instance,
) {
if (__DEV__ && parentProps[SUPPRESS_HYDRATION_WARNING] !== true) {
if (__DEV__) {
// TODO: warnForInsertedHydratedSuspense(parentInstance);
}
}
Expand Down
34 changes: 27 additions & 7 deletions packages/react-reconciler/src/ReactFiberHydrationContext.new.js
View file
Open in desktop
Expand Up @@ -148,28 +148,34 @@ function warnUnhydratedInstance(
) {
if (__DEV__) {
switch (returnFiber.tag) {
case HostRoot:
case HostRoot: {
didNotHydrateInstanceWithinContainer(
returnFiber.stateNode.containerInfo,
instance,
);
break;
case HostComponent:
}
case HostComponent: {
const isConcurrentMode = (returnFiber.mode & ConcurrentMode) !== NoMode;
didNotHydrateInstance(
returnFiber.type,
returnFiber.memoizedProps,
returnFiber.stateNode,
instance,
// TODO: Delete this argument when we remove the legacy root API.
isConcurrentMode,
);
break;
case SuspenseComponent:
}
case SuspenseComponent: {
const suspenseState: SuspenseState = returnFiber.memoizedState;
if (suspenseState.dehydrated !== null)
didNotHydrateInstanceWithinSuspenseInstance(
suspenseState.dehydrated,
instance,
);
break;
}
}
}
}
Expand Down Expand Up @@ -234,33 +240,44 @@ function warnNonhydratedInstance(returnFiber: Fiber, fiber: Fiber) {
const parentProps = returnFiber.memoizedProps;
const parentInstance = returnFiber.stateNode;
switch (fiber.tag) {
case HostComponent:
case HostComponent: {
const type = fiber.type;
const props = fiber.pendingProps;
const isConcurrentMode =
(returnFiber.mode & ConcurrentMode) !== NoMode;
didNotFindHydratableInstance(
parentType,
parentProps,
parentInstance,
type,
props,
// TODO: Delete this argument when we remove the legacy root API.
isConcurrentMode,
);
break;
case HostText:
}
case HostText: {
const text = fiber.pendingProps;
const isConcurrentMode =
(returnFiber.mode & ConcurrentMode) !== NoMode;
didNotFindHydratableTextInstance(
parentType,
parentProps,
parentInstance,
text,
// TODO: Delete this argument when we remove the legacy root API.
isConcurrentMode,
);
break;
case SuspenseComponent:
}
case SuspenseComponent: {
didNotFindHydratableSuspenseInstance(
parentType,
parentProps,
parentInstance,
);
break;
}
}
break;
}
Expand Down Expand Up @@ -476,10 +493,11 @@ function prepareToHydrateHostTextInstance(fiber: Fiber): boolean {
// hydration parent is the parent host component of this host text.
const returnFiber = hydrationParentFiber;
if (returnFiber !== null) {
const isConcurrentMode = (returnFiber.mode & ConcurrentMode) !== NoMode;
switch (returnFiber.tag) {
case HostRoot: {
const parentContainer = returnFiber.stateNode.containerInfo;
const isConcurrentMode =
(returnFiber.mode & ConcurrentMode) !== NoMode;
didNotMatchHydratedContainerTextInstance(
parentContainer,
textInstance,
Expand All @@ -493,6 +511,8 @@ function prepareToHydrateHostTextInstance(fiber: Fiber): boolean {
const parentType = returnFiber.type;
const parentProps = returnFiber.memoizedProps;
const parentInstance = returnFiber.stateNode;
const isConcurrentMode =
(returnFiber.mode & ConcurrentMode) !== NoMode;
didNotMatchHydratedTextInstance(
parentType,
parentProps,
Expand Down
34 changes: 27 additions & 7 deletions packages/react-reconciler/src/ReactFiberHydrationContext.old.js
View file
Open in desktop
Expand Up @@ -148,28 +148,34 @@ function warnUnhydratedInstance(
) {
if (__DEV__) {
switch (returnFiber.tag) {
case HostRoot:
case HostRoot: {
didNotHydrateInstanceWithinContainer(
returnFiber.stateNode.containerInfo,
instance,
);
break;
case HostComponent:
}
case HostComponent: {
const isConcurrentMode = (returnFiber.mode & ConcurrentMode) !== NoMode;
didNotHydrateInstance(
returnFiber.type,
returnFiber.memoizedProps,
returnFiber.stateNode,
instance,
// TODO: Delete this argument when we remove the legacy root API.
isConcurrentMode,
);
break;
case SuspenseComponent:
}
case SuspenseComponent: {
const suspenseState: SuspenseState = returnFiber.memoizedState;
if (suspenseState.dehydrated !== null)
didNotHydrateInstanceWithinSuspenseInstance(
suspenseState.dehydrated,
instance,
);
break;
}
}
}
}
Expand Down Expand Up @@ -234,33 +240,44 @@ function warnNonhydratedInstance(returnFiber: Fiber, fiber: Fiber) {
const parentProps = returnFiber.memoizedProps;
const parentInstance = returnFiber.stateNode;
switch (fiber.tag) {
case HostComponent:
case HostComponent: {
const type = fiber.type;
const props = fiber.pendingProps;
const isConcurrentMode =
(returnFiber.mode & ConcurrentMode) !== NoMode;
didNotFindHydratableInstance(
parentType,
parentProps,
parentInstance,
type,
props,
// TODO: Delete this argument when we remove the legacy root API.
isConcurrentMode,
);
break;
case HostText:
}
case HostText: {
const text = fiber.pendingProps;
const isConcurrentMode =
(returnFiber.mode & ConcurrentMode) !== NoMode;
didNotFindHydratableTextInstance(
parentType,
parentProps,
parentInstance,
text,
// TODO: Delete this argument when we remove the legacy root API.
isConcurrentMode,
);
break;
case SuspenseComponent:
}
case SuspenseComponent: {
didNotFindHydratableSuspenseInstance(
parentType,
parentProps,
parentInstance,
);
break;
}
}
break;
}
Expand Down Expand Up @@ -476,10 +493,11 @@ function prepareToHydrateHostTextInstance(fiber: Fiber): boolean {
// hydration parent is the parent host component of this host text.
const returnFiber = hydrationParentFiber;
if (returnFiber !== null) {
const isConcurrentMode = (returnFiber.mode & ConcurrentMode) !== NoMode;
switch (returnFiber.tag) {
case HostRoot: {
const parentContainer = returnFiber.stateNode.containerInfo;
const isConcurrentMode =
(returnFiber.mode & ConcurrentMode) !== NoMode;
didNotMatchHydratedContainerTextInstance(
parentContainer,
textInstance,
Expand All @@ -493,6 +511,8 @@ function prepareToHydrateHostTextInstance(fiber: Fiber): boolean {
const parentType = returnFiber.type;
const parentProps = returnFiber.memoizedProps;
const parentInstance = returnFiber.stateNode;
const isConcurrentMode =
(returnFiber.mode & ConcurrentMode) !== NoMode;
didNotMatchHydratedTextInstance(
parentType,
parentProps,
Expand Down