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

when float is enabled only push title and script as a single unit #25536

Merged
merged 5 commits into from
Oct 22, 2022
Merged

when float is enabled only push title and script as a single unit #25536

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2f241fd
when float is enabled only push title and script as a single unit
gnoff Oct 22, 2022
6e2f5e3
tests
gnoff Oct 22, 2022
1ec3723
restore title warnings on server render without float
gnoff Oct 22, 2022
3331989
remvoe gate
gnoff Oct 22, 2022
a36bde7
lint
gnoff Oct 22, 2022
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
3 changes: 1 addition & 2 deletions packages/react-dom-bindings/src/server/ReactDOMFloatServer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,9 +645,8 @@ export function resourcesFromElement(type: string, props: Props): boolean {
resources.headsMap.set(key, resource);
resources.headResources.add(resource);
}
return true;
}
return false;
return true;
}
case 'meta': {
let key, propertyPath;
Expand Down
194 changes: 169 additions & 25 deletions packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1294,21 +1294,102 @@ function pushStartMenuItem(
return null;
}

function pushStartTitle(
function pushTitle(
target: Array<Chunk | PrecomputedChunk>,
props: Object,
responseState: ResponseState,
): ReactNodeList {
if (__DEV__) {
const children = props.children;
const childForValidation =
Array.isArray(children) && children.length < 2
? children[0] || null
: children;
if (Array.isArray(children) && children.length > 1) {
console.error(
'A title element received an array with more than 1 element as children. ' +
'In browsers title Elements can only have Text Nodes as children. If ' +
'the children being rendered output more than a single text node in aggregate the browser ' +
'will display markup and comments as text in the title and hydration will likely fail and ' +
'fall back to client rendering',
);
} else if (
childForValidation != null &&
childForValidation.$$typeof != null
) {
console.error(
'A title element received a React element for children. ' +
'In the browser title Elements can only have Text Nodes as children. If ' +
'the children being rendered output more than a single text node in aggregate the browser ' +
'will display markup and comments as text in the title and hydration will likely fail and ' +
'fall back to client rendering',
);
} else if (
childForValidation != null &&
typeof childForValidation !== 'string' &&
typeof childForValidation !== 'number'
) {
console.error(
'A title element received a value that was not a string or number for children. ' +
'In the browser title Elements can only have Text Nodes as children. If ' +
'the children being rendered output more than a single text node in aggregate the browser ' +
'will display markup and comments as text in the title and hydration will likely fail and ' +
'fall back to client rendering',
);
}
}

if (enableFloat && resourcesFromElement('title', props)) {
// We have converted this link exclusively to a resource and no longer
// need to emit it
return null;
}
return pushTitleImpl(target, props, responseState);
}

return pushStartTitleImpl(target, props, responseState);
function pushTitleImpl(
target: Array<Chunk | PrecomputedChunk>,
props: Object,
responseState: ResponseState,
): null {
target.push(startChunkForTag('title'));

let children = null;
for (const propKey in props) {
if (hasOwnProperty.call(props, propKey)) {
const propValue = props[propKey];
if (propValue == null) {
continue;
}
switch (propKey) {
case 'children':
children = propValue;
break;
case 'dangerouslySetInnerHTML':
throw new Error(
'`dangerouslySetInnerHTML` does not make sense on <title>.',
);
// eslint-disable-next-line-no-fallthrough
default:
pushAttribute(target, responseState, propKey, propValue);
break;
}
}
}
target.push(endOfStartTag);

const child =
Array.isArray(children) && children.length < 2
? children[0] || null
: children;
if (typeof child === 'string' || typeof child === 'number') {
target.push(stringToChunk(escapeTextForBrowser(child)));
}
target.push(endTag1, stringToChunk('title'), endTag2);
return null;
}

function pushStartTitleImpl(
function pushStartTitle(
target: Array<Chunk | PrecomputedChunk>,
props: Object,
responseState: ResponseState,
Expand Down Expand Up @@ -1340,7 +1421,7 @@ function pushStartTitleImpl(
target.push(endOfStartTag);

if (__DEV__) {
const child =
const childForValidation =
Array.isArray(children) && children.length < 2
? children[0] || null
: children;
Expand All @@ -1352,7 +1433,10 @@ function pushStartTitleImpl(
'will display markup and comments as text in the title and hydration will likely fail and ' +
'fall back to client rendering',
);
} else if (child != null && child.$$typeof != null) {
} else if (
childForValidation != null &&
childForValidation.$$typeof != null
) {
console.error(
'A title element received a React element for children. ' +
'In the browser title Elements can only have Text Nodes as children. If ' +
Expand All @@ -1361,9 +1445,9 @@ function pushStartTitleImpl(
'fall back to client rendering',
);
} else if (
child != null &&
typeof child !== 'string' &&
typeof child !== 'number'
childForValidation != null &&
typeof childForValidation !== 'string' &&
typeof childForValidation !== 'number'
) {
console.error(
'A title element received a value that was not a string or number for children. ' +
Expand All @@ -1374,6 +1458,7 @@ function pushStartTitleImpl(
);
}
}

return children;
}

Expand Down Expand Up @@ -1410,12 +1495,12 @@ function pushStartHtml(
return pushStartGenericElement(target, props, tag, responseState);
}

function pushStartScript(
function pushScript(
target: Array<Chunk | PrecomputedChunk>,
props: Object,
responseState: ResponseState,
textEmbedded: boolean,
): ReactNodeList {
): null {
if (enableFloat && resourcesFromScript(props)) {
if (textEmbedded) {
// This link follows text but we aren't writing a tag. while not as efficient as possible we need
Expand All @@ -1427,7 +1512,61 @@ function pushStartScript(
return null;
}

return pushStartGenericElement(target, props, 'script', responseState);
return pushScriptImpl(target, props, responseState);
}

function pushScriptImpl(
target: Array<Chunk | PrecomputedChunk>,
props: Object,
responseState: ResponseState,
): null {
target.push(startChunkForTag('script'));

let children = null;
let innerHTML = null;
for (const propKey in props) {
if (hasOwnProperty.call(props, propKey)) {
const propValue = props[propKey];
if (propValue == null) {
continue;
}
switch (propKey) {
case 'children':
children = propValue;
break;
case 'dangerouslySetInnerHTML':
innerHTML = propValue;
break;
default:
pushAttribute(target, responseState, propKey, propValue);
break;
}
}
}
target.push(endOfStartTag);

if (__DEV__) {
if (children != null && typeof children !== 'string') {
const descriptiveStatement =
typeof children === 'number'
? 'a number for children'
: Array.isArray(children)
? 'an array for children'
: 'something unexpected for children';
console.error(
'A script element was rendered with %s. If script element has children it must be a single string.' +
' Consider using dangerouslySetInnerHTML or passing a plain string as children.',
descriptiveStatement,
);
}
}

pushInnerHTML(target, innerHTML, children);
if (typeof children === 'string') {
target.push(stringToChunk(encodeHTMLTextNode(children)));
}
target.push(endTag1, stringToChunk('script'), endTag2);
return null;
}

function pushStartGenericElement(
Expand Down Expand Up @@ -1703,11 +1842,15 @@ export function pushStartInstance(
case 'menuitem':
return pushStartMenuItem(target, props, responseState);
case 'title':
return pushStartTitle(target, props, responseState);
return enableFloat
? pushTitle(target, props, responseState)
: pushStartTitle(target, props, responseState);
case 'link':
return pushLink(target, props, responseState, textEmbedded);
case 'script':
return pushStartScript(target, props, responseState, textEmbedded);
return enableFloat
? pushScript(target, props, responseState, textEmbedded)
: pushStartGenericElement(target, props, type, responseState);
case 'meta':
return pushMeta(target, props, responseState, textEmbedded);
// Newline eating tags
Expand Down Expand Up @@ -1777,9 +1920,19 @@ export function pushEndInstance(
props: Object,
): void {
switch (type) {
// When float is on we expect title and script tags to always be pushed in
// a unit and never return children. when we end up pushing the end tag we
// want to ensure there is no extra closing tag pushed
case 'title':
case 'script': {
if (!enableFloat) {
break;
}
}
// Omitted close tags
// TODO: Instead of repeating this switch we could try to pass a flag from above.
// That would require returning a tuple. Which might be ok if it gets inlined.
// eslint-disable-next-line-no-fallthrough
case 'area':
case 'base':
case 'br':
Expand Down Expand Up @@ -2396,8 +2549,7 @@ export function writeInitialResources(

scripts.forEach(r => {
// should never be flushed already
pushStartGenericElement(target, r.props, 'script', responseState);
pushEndInstance(target, target, 'script', r.props);
pushScriptImpl(target, r.props, responseState);
r.flushed = true;
r.hint.flushed = true;
});
Expand All @@ -2415,11 +2567,7 @@ export function writeInitialResources(
headResources.forEach(r => {
switch (r.type) {
case 'title': {
pushStartTitleImpl(target, r.props, responseState);
if (typeof r.props.children === 'string') {
target.push(stringToChunk(escapeTextForBrowser(r.props.children)));
}
pushEndInstance(target, target, 'title', r.props);
pushTitleImpl(target, r.props, responseState);
break;
}
case 'meta': {
Expand Down Expand Up @@ -2516,11 +2664,7 @@ export function writeImmediateResources(
headResources.forEach(r => {
switch (r.type) {
case 'title': {
pushStartTitleImpl(target, r.props, responseState);
if (typeof r.props.children === 'string') {
target.push(stringToChunk(escapeTextForBrowser(r.props.children)));
}
pushEndInstance(target, target, 'title', r.props);
pushTitleImpl(target, r.props, responseState);
break;
}
case 'meta': {
Expand Down
Loading