Skip to content

[Float] handle non-void resources properly #25535

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

Closed
wants to merge 2 commits into from
Closed
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
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,11 @@ function encodeHTMLTextNode(text: string): string {
return escapeTextForBrowser(text);
}

// This const is returned when a push is fully consumed as a Resource. The runtime
// well compare it against the returned children and avoid pushing the end tag
opaque type ResourceSentinel = mixed;
export const RESOURCE_SENTINAL: ResourceSentinel = {};

const textSeparator = stringToPrecomputedChunk('<!-- -->');

export function pushTextInstance(
Expand Down Expand Up @@ -1155,7 +1160,7 @@ function pushMeta(
props: Object,
responseState: ResponseState,
textEmbedded: boolean,
): ReactNodeList {
): ReactNodeList | ResourceSentinel {
if (enableFloat && resourcesFromElement('meta', props)) {
if (textEmbedded) {
// This link follows text but we aren't writing a tag. while not as efficient as possible we need
Expand All @@ -1164,7 +1169,7 @@ function pushMeta(
}
// We have converted this link exclusively to a resource and no longer
// need to emit it
return null;
return RESOURCE_SENTINAL;
}

return pushSelfClosing(target, props, 'meta', responseState);
Expand All @@ -1175,7 +1180,7 @@ function pushLink(
props: Object,
responseState: ResponseState,
textEmbedded: boolean,
): ReactNodeList {
): ReactNodeList | ResourceSentinel {
if (enableFloat && resourcesFromLink(props)) {
if (textEmbedded) {
// This link follows text but we aren't writing a tag. while not as efficient as possible we need
Expand All @@ -1184,7 +1189,7 @@ function pushLink(
}
// We have converted this link exclusively to a resource and no longer
// need to emit it
return null;
return RESOURCE_SENTINAL;
}

return pushLinkImpl(target, props, responseState);
Expand Down Expand Up @@ -1298,11 +1303,11 @@ function pushStartTitle(
target: Array<Chunk | PrecomputedChunk>,
props: Object,
responseState: ResponseState,
): ReactNodeList {
): ReactNodeList | ResourceSentinel {
if (enableFloat && resourcesFromElement('title', props)) {
// We have converted this link exclusively to a resource and no longer
// need to emit it
return null;
return RESOURCE_SENTINAL;
}

return pushStartTitleImpl(target, props, responseState);
Expand Down Expand Up @@ -1415,7 +1420,7 @@ function pushStartScript(
props: Object,
responseState: ResponseState,
textEmbedded: boolean,
): ReactNodeList {
): ReactNodeList | ResourceSentinel {
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 @@ -1424,7 +1429,7 @@ function pushStartScript(
}
// We have converted this link exclusively to a resource and no longer
// need to emit it
return null;
return RESOURCE_SENTINAL;
}

return pushStartGenericElement(target, props, 'script', responseState);
Expand Down Expand Up @@ -1652,7 +1657,7 @@ export function pushStartInstance(
responseState: ResponseState,
formatContext: FormatContext,
textEmbedded: boolean,
): ReactNodeList {
): ReactNodeList | ResourceSentinel {
if (__DEV__) {
validateARIAProperties(type, props);
validateInputProperties(type, props);
Expand Down Expand Up @@ -1767,6 +1772,10 @@ export function pushStartInstance(
}
}

// function pushEndTitle(target: Array<Chunk | PrecomputedChunk>): void {
// if (enableFloat)
// }

const endTag1 = stringToPrecomputedChunk('</');
const endTag2 = stringToPrecomputedChunk('>');

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ export {
UNINITIALIZED_SUSPENSE_BOUNDARY_ID,
assignSuspenseBoundaryID,
makeId,
RESOURCE_SENTINAL,
pushStartInstance,
pushEndInstance,
pushStartCompletedSuspenseBoundary,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ describe('ReactDOMFizzServerBrowser', () => {
});

// https://github.com/facebook/react/pull/25534/files - fix transposed escape functions
// @gate enableFloat
// not gated on enableFloat because this is also the correct behavior when float is off
it('should encode title properly', async () => {
const stream = await ReactDOMFizzServer.renderToReadableStream(
<html>
Expand All @@ -499,7 +499,7 @@ describe('ReactDOMFizzServerBrowser', () => {

const result = await readResult(stream);
expect(result).toEqual(
'<!DOCTYPE html><html><head><title>foo</title></title></head><body>bar</body></html>',
'<!DOCTYPE html><html><head><title>foo</title></head><body>bar</body></html>',
);
});
});
26 changes: 26 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMFloat-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,32 @@ describe('ReactDOMFloat', () => {
);
});

// @gate enableFloat
it('does not emit closing tags in out of order position when rendering a non-void resource type', async () => {
const chunks = [];

writable.on('data', chunk => {
chunks.push(chunk);
});

await actIntoEmptyDocument(() => {
const {pipe} = ReactDOMFizzServer.renderToPipeableStream(
<>
<title>foo</title>
<html>
<body>bar</body>
</html>
<script async={true} src="foo" />
</>,
);
pipe(writable);
});
expect(chunks).toEqual([
'<!DOCTYPE html><html><script async="" src="foo"></script><title>foo</title><body>bar',
'</body></html>',
]);
});

describe('HostResource', () => {
// @gate enableFloat
it('warns when you update props to an invalid type', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,8 @@ export function makeId(

const RAW_TEXT = stringToPrecomputedChunk('RCTRawText');

export const RESOURCE_SENTINAL: mixed = {};

export function pushTextInstance(
target: Array<Chunk | PrecomputedChunk>,
text: string,
Expand Down
10 changes: 9 additions & 1 deletion packages/react-server/src/ReactFizzServer.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ import {
setCurrentlyRenderingBoundaryResourcesTarget,
createResources,
createBoundaryResources,
RESOURCE_SENTINAL,
} from './ReactServerFormatConfig';
import {
constructClassInstance,
Expand Down Expand Up @@ -694,7 +695,7 @@ function renderHostElement(
pushBuiltInComponentStackInDEV(task, type);
const segment = task.blockedSegment;

const children = pushStartInstance(
const childrenOrResource = pushStartInstance(
segment.chunks,
request.preamble,
type,
Expand All @@ -703,6 +704,13 @@ function renderHostElement(
segment.formatContext,
segment.lastPushedText,
);
if (enableFloat && childrenOrResource === RESOURCE_SENTINAL) {
// this push did not actually write to segment chunks because the element
// was a Resource. We pop the stack and return early.
popComponentStackInDEV(task);
return;
}
const children: ReactNodeList = (childrenOrResource: any);
segment.lastPushedText = false;
const prevContext = segment.formatContext;
segment.formatContext = getChildFormatContext(prevContext, type, props);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,3 +82,4 @@ export const createResources = $$$hostConfig.createResources;
export const createBoundaryResources = $$$hostConfig.createBoundaryResources;
export const setCurrentlyRenderingBoundaryResourcesTarget =
$$$hostConfig.setCurrentlyRenderingBoundaryResourcesTarget;
export const RESOURCE_SENTINAL = $$$hostConfig.RESOURCE_SENTINAL;