Skip to content

[Float] handle noscript context for Resources #25559

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.

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

Merged
merged 4 commits into from
Oct 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions packages/react-dom-bindings/src/server/ReactDOMFloatServer.js
Original file line number Diff line number Diff line change
Expand Up @@ -863,6 +863,10 @@ export function resourcesFromLink(props: Props): boolean {
}
}
if (props.onLoad || props.onError) {
// When a link has these props we can't treat it is a Resource but if we rendered it on the
// server it would look like a Resource in the rendered html (the onLoad/onError aren't emitted)
// Instead we expect the client to insert them rather than hydrate them which also guarantees
// that the onLoad and onError won't fire before the event handlers are attached
return true;
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 means we won't emit this element on the server. it's sort of a black hole where we created no resources but we also aren't going to render it as a component either. it will render as an insert on the client

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add this as a comment inline.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if it would make sense to emit it as a kind of low priority preload though. It seems a bit late to wait for hydration to actually load it off the network.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

well as you pointed out before, we can't know if it is actually loadable. this is a generic pathway for links so this could be some strange rel like "foo".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe known ones can still emit but also be inserted on the client. for instance preconnect and dns-prefetch and maybe even prefetch

}

Expand Down
128 changes: 108 additions & 20 deletions packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -274,15 +274,18 @@ type InsertionMode = 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7;
export type FormatContext = {
insertionMode: InsertionMode, // root/svg/html/mathml/table
selectedValue: null | string | Array<string>, // the selected value(s) inside a <select>, or null outside <select>
noscriptTagInScope: boolean,
};

function createFormatContext(
insertionMode: InsertionMode,
selectedValue: null | string,
noscriptTagInScope: boolean,
): FormatContext {
return {
insertionMode,
selectedValue,
noscriptTagInScope,
};
}

Expand All @@ -293,7 +296,7 @@ export function createRootFormatContext(namespaceURI?: string): FormatContext {
: namespaceURI === 'http://www.w3.org/1998/Math/MathML'
? MATHML_MODE
: ROOT_HTML_MODE;
return createFormatContext(insertionMode, null);
return createFormatContext(insertionMode, null, false);
}

export function getChildFormatContext(
Expand All @@ -302,38 +305,77 @@ export function getChildFormatContext(
props: Object,
): FormatContext {
switch (type) {
case 'noscript':
return createFormatContext(HTML_MODE, null, true);
case 'select':
return createFormatContext(
HTML_MODE,
props.value != null ? props.value : props.defaultValue,
parentContext.noscriptTagInScope,
);
case 'svg':
return createFormatContext(SVG_MODE, null);
return createFormatContext(
SVG_MODE,
null,
parentContext.noscriptTagInScope,
);
case 'math':
return createFormatContext(MATHML_MODE, null);
return createFormatContext(
MATHML_MODE,
null,
parentContext.noscriptTagInScope,
);
case 'foreignObject':
return createFormatContext(HTML_MODE, null);
return createFormatContext(
HTML_MODE,
null,
parentContext.noscriptTagInScope,
);
// Table parents are special in that their children can only be created at all if they're
// wrapped in a table parent. So we need to encode that we're entering this mode.
case 'table':
return createFormatContext(HTML_TABLE_MODE, null);
return createFormatContext(
HTML_TABLE_MODE,
null,
parentContext.noscriptTagInScope,
);
case 'thead':
case 'tbody':
case 'tfoot':
return createFormatContext(HTML_TABLE_BODY_MODE, null);
return createFormatContext(
HTML_TABLE_BODY_MODE,
null,
parentContext.noscriptTagInScope,
);
case 'colgroup':
return createFormatContext(HTML_COLGROUP_MODE, null);
return createFormatContext(
HTML_COLGROUP_MODE,
null,
parentContext.noscriptTagInScope,
);
case 'tr':
return createFormatContext(HTML_TABLE_ROW_MODE, null);
return createFormatContext(
HTML_TABLE_ROW_MODE,
null,
parentContext.noscriptTagInScope,
);
}
if (parentContext.insertionMode >= HTML_TABLE_MODE) {
// Whatever tag this was, it wasn't a table parent or other special parent, so we must have
// entered plain HTML again.
return createFormatContext(HTML_MODE, null);
return createFormatContext(
HTML_MODE,
null,
parentContext.noscriptTagInScope,
);
}
if (parentContext.insertionMode === ROOT_HTML_MODE) {
// We've emitted the root and is now in plain HTML mode.
return createFormatContext(HTML_MODE, null);
return createFormatContext(
HTML_MODE,
null,
parentContext.noscriptTagInScope,
);
}
return parentContext;
}
Expand Down Expand Up @@ -1155,8 +1197,13 @@ function pushBase(
props: Object,
responseState: ResponseState,
textEmbedded: boolean,
noscriptTagInScope: boolean,
): ReactNodeList {
if (enableFloat && resourcesFromElement('base', props)) {
if (
enableFloat &&
!noscriptTagInScope &&
resourcesFromElement('base', props)
) {
if (textEmbedded) {
// This link follows text but we aren't writing a tag. while not as efficient as possible we need
// to be safe and assume text will follow by inserting a textSeparator
Expand All @@ -1175,8 +1222,13 @@ function pushMeta(
props: Object,
responseState: ResponseState,
textEmbedded: boolean,
noscriptTagInScope: boolean,
): ReactNodeList {
if (enableFloat && resourcesFromElement('meta', props)) {
if (
enableFloat &&
!noscriptTagInScope &&
resourcesFromElement('meta', props)
) {
if (textEmbedded) {
// This link follows text but we aren't writing a tag. while not as efficient as possible we need
// to be safe and assume text will follow by inserting a textSeparator
Expand All @@ -1195,8 +1247,9 @@ function pushLink(
props: Object,
responseState: ResponseState,
textEmbedded: boolean,
noscriptTagInScope: boolean,
): ReactNodeList {
if (enableFloat && resourcesFromLink(props)) {
if (enableFloat && !noscriptTagInScope && resourcesFromLink(props)) {
if (textEmbedded) {
// This link follows text but we aren't writing a tag. while not as efficient as possible we need
// to be safe and assume text will follow by inserting a textSeparator
Expand Down Expand Up @@ -1318,6 +1371,7 @@ function pushTitle(
target: Array<Chunk | PrecomputedChunk>,
props: Object,
responseState: ResponseState,
noscriptTagInScope: boolean,
): ReactNodeList {
if (__DEV__) {
const children = props.children;
Expand Down Expand Up @@ -1359,7 +1413,11 @@ function pushTitle(
}
}

if (enableFloat && resourcesFromElement('title', props)) {
if (
enableFloat &&
!noscriptTagInScope &&
resourcesFromElement('title', props)
) {
// We have converted this link exclusively to a resource and no longer
// need to emit it
return null;
Expand Down Expand Up @@ -1520,8 +1578,9 @@ function pushScript(
props: Object,
responseState: ResponseState,
textEmbedded: boolean,
noscriptTagInScope: boolean,
): null {
if (enableFloat && resourcesFromScript(props)) {
if (enableFloat && !noscriptTagInScope && resourcesFromScript(props)) {
if (textEmbedded) {
// This link follows text but we aren't writing a tag. while not as efficient as possible we need
// to be safe and assume text will follow by inserting a textSeparator
Expand Down Expand Up @@ -1863,18 +1922,47 @@ export function pushStartInstance(
return pushStartMenuItem(target, props, responseState);
case 'title':
return enableFloat
? pushTitle(target, props, responseState)
? pushTitle(
target,
props,
responseState,
formatContext.noscriptTagInScope,
)
: pushStartTitle(target, props, responseState);
case 'link':
return pushLink(target, props, responseState, textEmbedded);
return pushLink(
target,
props,
responseState,
textEmbedded,
formatContext.noscriptTagInScope,
);
case 'script':
return enableFloat
? pushScript(target, props, responseState, textEmbedded)
? pushScript(
target,
props,
responseState,
textEmbedded,
formatContext.noscriptTagInScope,
)
: pushStartGenericElement(target, props, type, responseState);
case 'meta':
return pushMeta(target, props, responseState, textEmbedded);
return pushMeta(
target,
props,
responseState,
textEmbedded,
formatContext.noscriptTagInScope,
);
case 'base':
return pushBase(target, props, responseState, textEmbedded);
return pushBase(
target,
props,
responseState,
textEmbedded,
formatContext.noscriptTagInScope,
);
// Newline eating tags
case 'listing':
case 'pre': {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ export function createRootFormatContext(): FormatContext {
return {
insertionMode: HTML_MODE, // We skip the root mode because we don't want to emit the DOCTYPE in legacy mode.
selectedValue: null,
noscriptTagInScope: false,
};
}

Expand Down
Loading