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

Allow forms to skip hydration of hidden inputs #26735

Merged
merged 4 commits into from
May 1, 2023
+202 −209
Merged

Allow forms to skip hydration of hidden inputs #26735

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7b55c14
Move shouldSkip into canHydrate
sebmarkbage Apr 28, 2023
4cbdc87
Inline shouldSkip into canHydrate
sebmarkbage Apr 28, 2023
62feb18
Filter don't delete tail nodes of forms
sebmarkbage Apr 28, 2023
b2b497c
Allow hidden inputs as extra nodes that we don't match
sebmarkbage Apr 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
281 changes: 153 additions & 128 deletions packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ import {
enableHostSingletons,
enableTrustedTypesIntegration,
diffInCommitPhase,
enableFormActions,
} from 'shared/ReactFeatureFlags';
import {
HostComponent,
Expand Down Expand Up @@ -1038,160 +1039,182 @@ export function isHydratableText(text: string): boolean {
return text !== '';
}

export function shouldSkipHydratableForInstance(
export function canHydrateInstance(
instance: HydratableInstance,
type: string,
props: Props,
): boolean {
if (instance.nodeType !== ELEMENT_NODE) {
// This is a suspense boundary or Text node.
// Suspense Boundaries are never expected to be injected by 3rd parties. If we see one it should be matched
// and this is a hydration error.
// Text Nodes are also not expected to be injected by 3rd parties. This is less of a guarantee for <body>
// but it seems reasonable and conservative to reject this as a hydration error as well
return false;
} else if (
instance.nodeName.toLowerCase() !== type.toLowerCase() ||
isMarkedHoistable(instance)
) {
// We are either about to
return true;
} else {
// We have an Element with the right type.
inRootOrSingleton: boolean,
): null | Instance {
while (instance.nodeType === ELEMENT_NODE) {
const element: Element = (instance: any);
const anyProps = (props: any);

// We are going to try to exclude it if we can definitely identify it as a hoisted Node or if
// we can guess that the node is likely hoisted or was inserted by a 3rd party script or browser extension
// using high entropy attributes for certain types. This technique will fail for strange insertions like
// extension prepending <div> in the <body> but that already breaks before and that is an edge case.
switch (type) {
// case 'title':
//We assume all titles are matchable. You should only have one in the Document, at least in a hoistable scope
// and if you are a HostComponent with type title we must either be in an <svg> context or this title must have an `itemProp` prop.
case 'meta': {
// The only way to opt out of hoisting meta tags is to give it an itemprop attribute. We assume there will be
// not 3rd party meta tags that are prepended, accepting the cases where this isn't true because meta tags
// are usually only functional for SSR so even in a rare case where we did bind to an injected tag the runtime
// implications are minimal
if (!element.hasAttribute('itemprop')) {
// This is a Hoistable
return true;
}
break;
}
case 'link': {
// Links come in many forms and we do expect 3rd parties to inject them into <head> / <body>. We exclude known resources
// and then use high-entroy attributes like href which are almost always used and almost always unique to filter out unlikely
// matches.
const rel = element.getAttribute('rel');
if (rel === 'stylesheet' && element.hasAttribute('data-precedence')) {
// This is a stylesheet resource
return true;
} else if (
rel !== anyProps.rel ||
element.getAttribute('href') !==
(anyProps.href == null ? null : anyProps.href) ||
element.getAttribute('crossorigin') !==
(anyProps.crossOrigin == null ? null : anyProps.crossOrigin) ||
element.getAttribute('title') !==
(anyProps.title == null ? null : anyProps.title)
if (element.nodeName.toLowerCase() !== type.toLowerCase()) {
if (!inRootOrSingleton || !enableHostSingletons) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need the host singleton check here? with flag is off we always hit this branch and with it on we skip it at the root level (and at the singleton level) but practically speaking we'll never be at root or singleton level because we'll always be in a form when this counts.

Seems like we can avoid the check altogether and always verify based on nodeName === 'input' or we can just use the inRootOrSingleton check and rely on the fact that when singleton flag is off it turn into an inRoot check

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 think this was just an oversight before this PR that this code should've been just completely eliminated with the flag off. Basically there's no loop here without it since it always exists.

// Usually we error for mismatched tags.
if (
enableFormActions &&
element.nodeName === 'INPUT' &&
(element: any).type === 'hidden'
) {
// rel + href should usually be enough to uniquely identify a link however crossOrigin can vary for rel preconnect
// and title could vary for rel alternate
return true;
// If we have extra hidden inputs, we don't mismatch. This allows us to embed
// extra form data in the original form.
} else {
return null;
}
break;
}
case 'style': {
// Styles are hard to match correctly. We can exclude known resources but otherwise we accept the fact that a non-hoisted style tags
// in <head> or <body> are likely never going to be unmounted given their position in the document and the fact they likely hold global styles
if (element.hasAttribute('data-precedence')) {
// This is a style resource
return true;
}
break;
// In root or singleton parents we skip past mismatched instances.
} else if (!inRootOrSingleton || !enableHostSingletons) {
// Match
if (
enableFormActions &&
type === 'input' &&
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This could just go into the switch below if we didn't bother checking the inRootOrSingleton context. Which probably makes sense since the other rules really apply to all context. It's just maybe worth for perf - especially checking isMarkedHoistable unnecessarily.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah maybe we just ignore the root/singleton context. Questioning that in my other comment

(element: any).type === 'hidden' &&
anyProps.type !== 'hidden'
) {
// Skip past hidden inputs unless that's what we're looking for. This allows us
// embed extra form data in the original form.
} else {
return element;
}
case 'script': {
// Scripts are a little tricky, we exclude known resources and then similar to links try to use high-entropy attributes
// to reject poor matches. One challenge with scripts are inline scripts. We don't attempt to check text content which could
// in theory lead to a hydration error later if a 3rd party injected an inline script before the React rendered nodes.
// Falling back to client rendering if this happens should be seemless though so we will try this hueristic and revisit later
// if we learn it is problematic
const srcAttr = element.getAttribute('src');
if (
srcAttr &&
element.hasAttribute('async') &&
!element.hasAttribute('itemprop')
) {
// This is an async script resource
return true;
} else if (
srcAttr !== (anyProps.src == null ? null : anyProps.src) ||
element.getAttribute('type') !==
(anyProps.type == null ? null : anyProps.type) ||
element.getAttribute('crossorigin') !==
(anyProps.crossOrigin == null ? null : anyProps.crossOrigin)
) {
// This script is for a different src
return true;
} else if (isMarkedHoistable(element)) {
// We've already claimed this as a hoistable which isn't hydrated this way so we skip past it.
} else {
// We have an Element with the right type.

// We are going to try to exclude it if we can definitely identify it as a hoisted Node or if
// we can guess that the node is likely hoisted or was inserted by a 3rd party script or browser extension
// using high entropy attributes for certain types. This technique will fail for strange insertions like
// extension prepending <div> in the <body> but that already breaks before and that is an edge case.
switch (type) {
// case 'title':
//We assume all titles are matchable. You should only have one in the Document, at least in a hoistable scope
// and if you are a HostComponent with type title we must either be in an <svg> context or this title must have an `itemProp` prop.
case 'meta': {
// The only way to opt out of hoisting meta tags is to give it an itemprop attribute. We assume there will be
// not 3rd party meta tags that are prepended, accepting the cases where this isn't true because meta tags
// are usually only functional for SSR so even in a rare case where we did bind to an injected tag the runtime
// implications are minimal
if (!element.hasAttribute('itemprop')) {
// This is a Hoistable
break;
}
return element;
}
case 'link': {
// Links come in many forms and we do expect 3rd parties to inject them into <head> / <body>. We exclude known resources
// and then use high-entroy attributes like href which are almost always used and almost always unique to filter out unlikely
// matches.
const rel = element.getAttribute('rel');
if (rel === 'stylesheet' && element.hasAttribute('data-precedence')) {
// This is a stylesheet resource
break;
} else if (
rel !== anyProps.rel ||
element.getAttribute('href') !==
(anyProps.href == null ? null : anyProps.href) ||
element.getAttribute('crossorigin') !==
(anyProps.crossOrigin == null ? null : anyProps.crossOrigin) ||
element.getAttribute('title') !==
(anyProps.title == null ? null : anyProps.title)
) {
// rel + href should usually be enough to uniquely identify a link however crossOrigin can vary for rel preconnect
// and title could vary for rel alternate
break;
}
return element;
}
case 'style': {
// Styles are hard to match correctly. We can exclude known resources but otherwise we accept the fact that a non-hoisted style tags
// in <head> or <body> are likely never going to be unmounted given their position in the document and the fact they likely hold global styles
if (element.hasAttribute('data-precedence')) {
// This is a style resource
break;
}
return element;
}
case 'script': {
// Scripts are a little tricky, we exclude known resources and then similar to links try to use high-entropy attributes
// to reject poor matches. One challenge with scripts are inline scripts. We don't attempt to check text content which could
// in theory lead to a hydration error later if a 3rd party injected an inline script before the React rendered nodes.
// Falling back to client rendering if this happens should be seemless though so we will try this hueristic and revisit later
// if we learn it is problematic
const srcAttr = element.getAttribute('src');
if (
srcAttr &&
element.hasAttribute('async') &&
!element.hasAttribute('itemprop')
) {
// This is an async script resource
break;
} else if (
srcAttr !== (anyProps.src == null ? null : anyProps.src) ||
element.getAttribute('type') !==
(anyProps.type == null ? null : anyProps.type) ||
element.getAttribute('crossorigin') !==
(anyProps.crossOrigin == null ? null : anyProps.crossOrigin)
) {
// This script is for a different src
break;
}
return element;
}
default: {
// We have excluded the most likely cases of mismatch between hoistable tags, 3rd party script inserted tags,
// and browser extension inserted tags. While it is possible this is not the right match it is a decent hueristic
// that should work in the vast majority of cases.
return element;
}
break;
}
}
// We have excluded the most likely cases of mismatch between hoistable tags, 3rd party script inserted tags,
// and browser extension inserted tags. While it is possible this is not the right match it is a decent hueristic
// that should work in the vast majority of cases.
return false;
}
}

export function shouldSkipHydratableForTextInstance(
instance: HydratableInstance,
): boolean {
return instance.nodeType === ELEMENT_NODE;
}

export function shouldSkipHydratableForSuspenseInstance(
instance: HydratableInstance,
): boolean {
return instance.nodeType === ELEMENT_NODE;
}

export function canHydrateInstance(
instance: HydratableInstance,
type: string,
props: Props,
): null | Instance {
if (
instance.nodeType !== ELEMENT_NODE ||
instance.nodeName.toLowerCase() !== type.toLowerCase()
) {
return null;
} else {
return ((instance: any): Instance);
const nextInstance = getNextHydratableSibling(element);
if (nextInstance === null) {
break;
}
instance = nextInstance;
}
// This is a suspense boundary or Text node or we got the end.
// Suspense Boundaries are never expected to be injected by 3rd parties. If we see one it should be matched
// and this is a hydration error.
// Text Nodes are also not expected to be injected by 3rd parties. This is less of a guarantee for <body>
// but it seems reasonable and conservative to reject this as a hydration error as well
return null;
}

export function canHydrateTextInstance(
instance: HydratableInstance,
text: string,
inRootOrSingleton: boolean,
): null | TextInstance {
// Empty strings are not parsed by HTML so there won't be a correct match here.
if (text === '') return null;

if (instance.nodeType !== TEXT_NODE) {
// Empty strings are not parsed by HTML so there won't be a correct match here.
return null;
while (instance.nodeType !== TEXT_NODE) {
if (!inRootOrSingleton || !enableHostSingletons) {
return null;
}
const nextInstance = getNextHydratableSibling(instance);
if (nextInstance === null) {
return null;
}
instance = nextInstance;
Comment on lines +1192 to +1199
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this may be a flaw already but if the hydratable instance is a suspense node but we're trying to hydrate text and we're in the root or a singleton then we'll call getNextHydratableSibling on the comment node which is incorrect. If hydratable is a comment I think we need to return null here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At the same time, does it matter? It's going to be a hydration error anyway, most likely, at least when the Suspense boundary doesn't find its node.

Arguably this is more resilient to non-suspense boundary comments that get inserted. I'm hesitant to "fix" it for that reason. Also, note that this is the same as the previous case. Only in inRootOrSingleton scope.

Copy link
Collaborator

Choose a reason for hiding this comment

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

makes sense, marked approved

}
// This has now been refined to a text node.
return ((instance: any): TextInstance);
}

export function canHydrateSuspenseInstance(
instance: HydratableInstance,
inRootOrSingleton: boolean,
): null | SuspenseInstance {
if (instance.nodeType !== COMMENT_NODE) {
return null;
while (instance.nodeType !== COMMENT_NODE) {
if (!inRootOrSingleton || !enableHostSingletons) {
return null;
}
const nextInstance = getNextHydratableSibling(instance);
if (nextInstance === null) {
return null;
}
instance = nextInstance;
}
// This has now been refined to a suspense node.
return ((instance: any): SuspenseInstance);
Expand Down Expand Up @@ -1416,12 +1439,14 @@ export function commitHydratedSuspenseInstance(
retryIfBlockedOn(suspenseInstance);
}

// @TODO remove this function once float lands and hydrated tail nodes
// are controlled by HostSingleton fibers
export function shouldDeleteUnhydratedTailInstances(
parentType: string,
): boolean {
return parentType !== 'head' && parentType !== 'body';
return (
(enableHostSingletons ||
(parentType !== 'head' && parentType !== 'body')) &&
(!enableFormActions || parentType !== 'form')
);
Comment on lines +1445 to +1449
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function is doing double duty as the signal for not clearing forms and as a signal to not clear head and body when singletons flag is turned off. We should make it two separate functions so it is clearer we can delete the legacy one once the singletons flag lands

}

export function didNotMatchHydratedContainerTextInstance(
Expand Down
37 changes: 37 additions & 0 deletions packages/react-dom/src/__tests__/ReactServerRenderingHydration-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -695,4 +695,41 @@ describe('ReactDOMServerHydration', () => {
);
}
});

// @gate enableFormActions
it('allows rendering extra hidden inputs in a form', async () => {
const element = document.createElement('div');
element.innerHTML =
'<form>' +
'<input type="hidden" /><input type="hidden" name="a" value="A" />' +
'<input type="hidden" /><input type="submit" name="b" value="B" />' +
'<input type="hidden" /><button name="c" value="C"></button>' +
'<input type="hidden" />' +
'</form>';
const form = element.firstChild;
const ref = React.createRef();
const a = React.createRef();
const b = React.createRef();
const c = React.createRef();
await act(async () => {
ReactDOMClient.hydrateRoot(
element,
<form ref={ref}>
<input type="hidden" name="a" value="A" ref={a} />
<input type="submit" name="b" value="B" ref={b} />
<button name="c" value="C" ref={c} />
</form>,
);
});

// The content should not have been client rendered.
expect(ref.current).toBe(form);

expect(a.current.name).toBe('a');
expect(a.current.value).toBe('A');
expect(b.current.name).toBe('b');
expect(b.current.value).toBe('B');
expect(c.current.name).toBe('c');
expect(c.current.value).toBe('C');
});
});
3 changes: 0 additions & 3 deletions packages/react-reconciler/src/ReactFiberConfigWithNoHydration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,6 @@ export const getNextHydratableSibling = shim;
export const getFirstHydratableChild = shim;
export const getFirstHydratableChildWithinContainer = shim;
export const getFirstHydratableChildWithinSuspenseInstance = shim;
export const shouldSkipHydratableForInstance = shim;
export const shouldSkipHydratableForTextInstance = shim;
export const shouldSkipHydratableForSuspenseInstance = shim;
export const canHydrateInstance = shim;
export const canHydrateTextInstance = shim;
export const canHydrateSuspenseInstance = shim;
Expand Down
Loading