Skip to content

[Flight] Encode server rendered host components as array tuples #18273

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

Merged
merged 1 commit into from
Mar 11, 2020
Merged

[Flight] Encode server rendered host components as array tuples #18273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 2 additions & 4 deletions fixtures/flight-browser/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,7 @@ <h1>Flight Example</h1>

let model = {
title: <Title />,
content: {
__html: <HTML />,
}
content: <HTML />,
};

let stream = ReactFlightDOMServer.renderToReadableStream(model);
Expand Down Expand Up @@ -90,7 +88,7 @@ <h1>Flight Example</h1>
<Suspense fallback="...">
<h1>{model.title}</h1>
</Suspense>
<div dangerouslySetInnerHTML={model.content} />
{model.content}
</div>;
}

Expand Down
4 changes: 1 addition & 3 deletions fixtures/flight/server/handler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,7 @@ function HTML() {
module.exports = function(req, res) {
res.setHeader('Access-Control-Allow-Origin', '*');
let model = {
content: {
__html: <HTML />,
},
content: <HTML />,
};
ReactFlightDOMServer.pipeToNodeWritable(model, res);
};
2 changes: 1 addition & 1 deletion fixtures/flight/src/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import React, {Suspense} from 'react';

function Content({data}) {
return <p dangerouslySetInnerHTML={data.model.content} />;
return data.model.content;
}

function App({data}) {
Expand Down
89 changes: 73 additions & 16 deletions packages/react-client/src/ReactFlightClient.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
* @flow
*/

import {REACT_ELEMENT_TYPE} from 'shared/ReactSymbols';

export type ReactModelRoot<T> = {|
model: T,
|};
Expand All @@ -19,6 +21,8 @@ export type JSONValue =
| {[key: string]: JSONValue}
| Array<JSONValue>;

const isArray = Array.isArray;

const PENDING = 0;
const RESOLVED = 1;
const ERRORED = 2;
Expand Down Expand Up @@ -141,28 +145,81 @@ function definePendingProperty(
});
}

function createElement(type, key, props): React$Element<any> {
const element: any = {
// This tag allows us to uniquely identify this as a React Element
$$typeof: REACT_ELEMENT_TYPE,

// Built-in properties that belong on the element
type: type,
key: key,
ref: null,
props: props,

// Record the component responsible for creating this element.
_owner: null,
};
if (__DEV__) {
// We don't really need to add any of these but keeping them for good measure.
// Unfortunately, _store is enumerable in jest matchers so for equality to
// work, I need to keep it or make _store non-enumerable in the other file.
element._store = {};
Object.defineProperty(element._store, 'validated', {
configurable: false,
enumerable: false,
writable: true,
value: true, // This element has already been validated on the server.
});
Object.defineProperty(element, '_self', {
configurable: false,
enumerable: false,
writable: false,
value: null,
});
Object.defineProperty(element, '_source', {
configurable: false,
enumerable: false,
writable: false,
value: null,
});
}
return element;
}

export function parseModelFromJSON(
response: Response,
targetObj: Object,
key: string,
value: JSONValue,
): any {
if (typeof value === 'string' && value[0] === '$') {
if (value[1] === '$') {
// This was an escaped string value.
return value.substring(1);
} else {
let id = parseInt(value.substring(1), 16);
let chunks = response.chunks;
let chunk = chunks.get(id);
if (!chunk) {
chunk = createPendingChunk();
chunks.set(id, chunk);
} else if (chunk.status === RESOLVED) {
return chunk.value;
): mixed {
if (typeof value === 'string') {
if (value[0] === '$') {
if (value === '$') {
return REACT_ELEMENT_TYPE;
} else if (value[1] === '$' || value[1] === '@') {
// This was an escaped string value.
return value.substring(1);
} else {
let id = parseInt(value.substring(1), 16);
let chunks = response.chunks;
let chunk = chunks.get(id);
if (!chunk) {
chunk = createPendingChunk();
chunks.set(id, chunk);
} else if (chunk.status === RESOLVED) {
return chunk.value;
}
definePendingProperty(targetObj, key, chunk);
return undefined;
}
definePendingProperty(targetObj, key, chunk);
return undefined;
}
}
if (isArray(value)) {
let tuple: [mixed, mixed, mixed, mixed] = (value: any);
if (tuple[0] === REACT_ELEMENT_TYPE) {
// TODO: Consider having React just directly accept these arrays as elements.
// Or even change the ReactElement type to be an array.
return createElement(tuple[1], tuple[2], tuple[3]);
Comment on lines +220 to +222
Copy link
Member

Choose a reason for hiding this comment

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

With that change we'd just be able to pass arrays through without deserializing, right? Though we'd still call this function on the props element, right? Or I guess those are flattened(?). The overall architecture makes sense but i'm still catching up on the details of each stage in the expansion, serialization, deserialization, and rendering.

Copy link
Collaborator Author

@sebmarkbage sebmarkbage Mar 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

There is still a deserialization step for the strings. String values starting with $ have special meaning so we need to deserialize those but we also need to unescape the user space strings that start with $.

JSON.parse materializes all objects and arrays in the JSON and then traverses over them by mutating them with new values. Which we'd still need to do to replace "$" with the symbol and replace references to reused or future values (i.e. "$12345" to their materialized value).

The benefit would be that we'd reuse the array already created by JSON.parse and just mutate the first slot from a string to a symbol.

We need the symbol to avoid the XSS potential in #3473.

The problem is that if this value gets passed into a render function and then returned from the render, we don't know if that came from Flight on the server or user provided JSON.

props: { userdata: ["$", "script", {children: "alert('p0wned')"}] }

So I solve this by escaping the "$" to "$$" in user provided data. At some point, I need to unescape it though so code can read from it. That's where deserialization comes in.

It's also used for references and I expect us to use it for more things.

In the raw Flight streaming model, this is done using the second argument to JSON.parse so it's kind of inline with the parser which (I hope) is very fast.

The issue with the Relay layering is that we currently have to do a second traversal. We could possibly avoid that if we were hooked in somehow to the same JSON.parse that Relay uses or any other pre-existing traversals.

Copy link
Member

@josephsavona josephsavona Mar 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

TIL that JSON.parse takes a second argument, i've never had a use-case for that but this is a great one.

The issue with the Relay layering is that we currently have to do a second traversal. We could possibly avoid that if we were hooked in somehow to the same JSON.parse that Relay uses or any other pre-existing traversals.

Cool. Note that Relay doesn't directly call JSON.parse - that's up to the user-provided network layer - so it should be possible for us to integrate the Flight deserialization into our internal network layer (and for folks in OSS to do the same) to avoid a double traversal.

}
}
return value;
Expand Down
4 changes: 2 additions & 2 deletions packages/react-flight-dom-relay/src/ReactFlightDOMRelayClient.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,11 @@ function parseModel(response, targetObj, key, value) {
if (typeof value === 'object' && value !== null) {
if (Array.isArray(value)) {
for (let i = 0; i < value.length; i++) {
value[i] = parseModel(response, value, '' + i, value[i]);
(value: any)[i] = parseModel(response, value, '' + i, value[i]);
}
} else {
for (let innerKey in value) {
value[innerKey] = parseModel(
(value: any)[innerKey] = parseModel(
response,
value,
innerKey,
Expand Down
43 changes: 40 additions & 3 deletions packages/react-flight-dom-webpack/src/__tests__/ReactFlightDOM-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,12 @@ describe('ReactFlightDOM', () => {
let result = ReactFlightDOMClient.readFromReadableStream(readable);
await waitForSuspense(() => {
expect(result.model).toEqual({
html: '<div><span>hello</span><span>world</span></div>',
html: (
<div>
<span>hello</span>
<span>world</span>
</div>
),
});
});
});
Expand Down Expand Up @@ -120,7 +125,7 @@ describe('ReactFlightDOM', () => {

// View
function Message({result}) {
return <p dangerouslySetInnerHTML={{__html: result.model.html}} />;
return <section>{result.model.html}</section>;
}
function App({result}) {
return (
Expand All @@ -140,7 +145,7 @@ describe('ReactFlightDOM', () => {
root.render(<App result={result} />);
});
expect(container.innerHTML).toBe(
'<p><div><span>hello</span><span>world</span></div></p>',
'<section><div><span>hello</span><span>world</span></div></section>',
);
});

Expand Down Expand Up @@ -176,6 +181,38 @@ describe('ReactFlightDOM', () => {
expect(container.innerHTML).toBe('<p>$1</p>');
});

it.experimental('should not get confused by @', async () => {
let {Suspense} = React;

// Model
function RootModel() {
return {text: '@div'};
}

// View
function Message({result}) {
return <p>{result.model.text}</p>;
}
function App({result}) {
return (
<Suspense fallback={<h1>Loading...</h1>}>
<Message result={result} />
</Suspense>
);
}

let {writable, readable} = getTestStream();
ReactFlightDOMServer.pipeToNodeWritable(<RootModel />, writable);
let result = ReactFlightDOMClient.readFromReadableStream(readable);

let container = document.createElement('div');
let root = ReactDOM.createRoot(container);
await act(async () => {
root.render(<App result={result} />);
});
expect(container.innerHTML).toBe('<p>@div</p>');
});

it.experimental('should progressively reveal chunks', async () => {
let {Suspense} = React;

Expand Down
7 changes: 6 additions & 1 deletion packages/react-flight-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,12 @@ describe('ReactFlightDOMBrowser', () => {
let result = ReactFlightDOMClient.readFromReadableStream(stream);
await waitForSuspense(() => {
expect(result.model).toEqual({
html: '<div><span>hello</span><span>world</span></div>',
html: (
<div>
<span>hello</span>
<span>world</span>
</div>
),
});
});
});
Expand Down
12 changes: 0 additions & 12 deletions packages/react-server/src/ReactDOMServerFormatConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@

import {convertStringToBuffer} from 'react-server/src/ReactServerStreamConfig';

import {renderToStaticMarkup} from 'react-dom/server';

export function formatChunkAsString(type: string, props: Object): string {
let str = '<' + type + '>';
if (typeof props.children === 'string') {
Expand All @@ -23,13 +21,3 @@ export function formatChunkAsString(type: string, props: Object): string {
export function formatChunk(type: string, props: Object): Uint8Array {
return convertStringToBuffer(formatChunkAsString(type, props));
}

export function renderHostChildrenToString(
children: React$Element<any>,
): string {
// TODO: This file is used to actually implement a server renderer
// so we can't actually reference the renderer here. Instead, we
// should replace this method with a reference to Fizz which
// then uses this file to implement the server renderer.
return renderToStaticMarkup(children);
}
10 changes: 7 additions & 3 deletions packages/react-server/src/ReactFlightServer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {
processModelChunk,
processErrorChunk,
} from './ReactFlightServerConfig';
import {renderHostChildrenToString} from './ReactServerFormatConfig';

import {REACT_ELEMENT_TYPE} from 'shared/ReactSymbols';

type ReactJSONValue =
Expand Down Expand Up @@ -88,7 +88,7 @@ function attemptResolveModelComponent(element: React$Element<any>): ReactModel {
return type(props);
} else if (typeof type === 'string') {
// This is a host element. E.g. HTML.
return renderHostChildrenToString(element);
return [REACT_ELEMENT_TYPE, type, element.key, element.props];
} else {
throw new Error('Unsupported type.');
}
Expand Down Expand Up @@ -119,7 +119,7 @@ function serializeIDRef(id: number): string {
function escapeStringValue(value: string): string {
if (value[0] === '$') {
// We need to escape $ prefixed strings since we use that to encode
// references to IDs.
// references to IDs and as a special symbol value.
return '$' + value;
} else {
return value;
Expand All @@ -134,6 +134,10 @@ export function resolveModelToJSON(
return escapeStringValue(value);
}

if (value === REACT_ELEMENT_TYPE) {
return '$';
}

while (
typeof value === 'object' &&
value !== null &&
Expand Down
2 changes: 0 additions & 2 deletions packages/react-server/src/forks/ReactServerFormatConfig.custom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,5 +28,3 @@ export opaque type Destination = mixed; // eslint-disable-line no-undef

export const formatChunkAsString = $$$hostConfig.formatChunkAsString;
export const formatChunk = $$$hostConfig.formatChunk;
export const renderHostChildrenToString =
$$$hostConfig.renderHostChildrenToString;