Skip to content

[Flight] Encode references to existing objects by property path #28996

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 2 commits into from
May 9, 2024
Merged

[Flight] Encode references to existing objects by property path #28996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1dba675
Encode references to existing objects by property path
sebmarkbage May 6, 2024
ff3a3c1
Test stream deduping
sebmarkbage May 6, 2024
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
43 changes: 27 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 @@ -680,6 +680,7 @@ function createModelResolver<T>(
cyclic: boolean,
response: Response,
map: (response: Response, model: any) => T,
path: Array<string>,
): (value: any) => void {
let blocked;
if (initializingChunkBlockedModel) {
Expand All @@ -694,6 +695,9 @@ function createModelResolver<T>(
};
}
return value => {
for (let i = 1; i < path.length; i++) {
value = value[path[i]];
}
parentObject[key] = map(response, value);

// If this is the root object for a model reference, where `blocked.value`
Expand Down Expand Up @@ -752,11 +756,13 @@ function createServerReferenceProxy<A: Iterable<any>, T>(

function getOutlinedModel<T>(
response: Response,
id: number,
reference: string,
parentObject: Object,
key: string,
map: (response: Response, model: any) => T,
): T {
const path = reference.split(':');
const id = parseInt(path[0], 16);
const chunk = getChunk(response, id);
switch (chunk.status) {
case RESOLVED_MODEL:
Expand All @@ -769,7 +775,11 @@ function getOutlinedModel<T>(
// The status might have changed after initialization.
switch (chunk.status) {
case INITIALIZED:
const chunkValue = map(response, chunk.value);
let value = chunk.value;
for (let i = 1; i < path.length; i++) {
value = value[path[i]];
}
const chunkValue = map(response, value);
if (__DEV__ && chunk._debugInfo) {
// If we have a direct reference to an object that was rendered by a synchronous
// server component, it might have some debug info about how it was rendered.
Expand Down Expand Up @@ -809,6 +819,7 @@ function getOutlinedModel<T>(
chunk.status === CYCLIC,
response,
map,
path,
),
createModelReject(parentChunk),
);
Expand Down Expand Up @@ -893,10 +904,10 @@ function parseModelString(
}
case 'F': {
// Server Reference
const id = parseInt(value.slice(2), 16);
const ref = value.slice(2);
return getOutlinedModel(
response,
id,
ref,
parentObject,
key,
createServerReferenceProxy,
Expand All @@ -916,39 +927,39 @@ function parseModelString(
}
case 'Q': {
// Map
const id = parseInt(value.slice(2), 16);
return getOutlinedModel(response, id, parentObject, key, createMap);
const ref = value.slice(2);
return getOutlinedModel(response, ref, parentObject, key, createMap);
}
case 'W': {
// Set
const id = parseInt(value.slice(2), 16);
return getOutlinedModel(response, id, parentObject, key, createSet);
const ref = value.slice(2);
return getOutlinedModel(response, ref, parentObject, key, createSet);
}
case 'B': {
// Blob
if (enableBinaryFlight) {
const id = parseInt(value.slice(2), 16);
return getOutlinedModel(response, id, parentObject, key, createBlob);
const ref = value.slice(2);
return getOutlinedModel(response, ref, parentObject, key, createBlob);
}
return undefined;
}
case 'K': {
// FormData
const id = parseInt(value.slice(2), 16);
const ref = value.slice(2);
return getOutlinedModel(
response,
id,
ref,
parentObject,
key,
createFormData,
);
}
case 'i': {
// Iterator
const id = parseInt(value.slice(2), 16);
const ref = value.slice(2);
return getOutlinedModel(
response,
id,
ref,
parentObject,
key,
extractIterator,
Expand Down Expand Up @@ -1000,8 +1011,8 @@ function parseModelString(
}
default: {
// We assume that anything else is a reference ID.
const id = parseInt(value.slice(1), 16);
return getOutlinedModel(response, id, parentObject, key, createModel);
const ref = value.slice(1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: To avoid confusion, I would prefer if we called this reference and not ref (here, as well as in the other switch cases), since we also use ref in this file to refer to the actual element ref. getOutlinedModel also calls the param reference.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is mainly to avoid all of these calls turning into multi-line.

return getOutlinedModel(response, ref, parentObject, key, createModel);
}
}
}
Expand Down
51 changes: 50 additions & 1 deletion packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ describe('ReactFlightDOMEdge', () => {
const [stream1, stream2] = passThrough(stream).tee();

const serializedContent = await readResult(stream1);
expect(serializedContent.length).toBeLessThan(400);
expect(serializedContent.length).toBeLessThan(470);

const result = await ReactServerDOMClient.createFromReadableStream(
stream2,
Expand Down Expand Up @@ -543,6 +543,55 @@ describe('ReactFlightDOMEdge', () => {
expect(await iterator.next()).toEqual({value: undefined, done: true});
});

// @gate enableFlightReadableStream
it('should ideally dedupe objects inside async iterables but does not yet', async () => {
const obj = {
this: {is: 'a large objected'},
with: {many: 'properties in it'},
};
const iterable = {
async *[Symbol.asyncIterator]() {
for (let i = 0; i < 30; i++) {
yield obj;
}
},
};

const stream = ReactServerDOMServer.renderToReadableStream({
iterable,
});
const [stream1, stream2] = passThrough(stream).tee();

const serializedContent = await readResult(stream1);
// TODO: Ideally streams should dedupe objects but because we never outline the objects
// they end up not having a row to reference them nor any of its nested objects.
// expect(serializedContent.length).toBeLessThan(400);
Copy link
Collaborator Author

@sebmarkbage sebmarkbage May 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Add a test to show case this. This used to work by the second row getting outlined.

Because every "chunk/entry" inside a stream has the same ID we can't refer to those objects. Only the stream object itself (ReadableStream or AsyncIterable) has a row ID.

This means we can't dedupe the object at the root of a stream row. Since we don't have a reference to that object we also can't dedupe the nested object via the parent neither. Leading to duplicating all these objects.

It seems unfortunate to outline every chunk/entry in a stream just in case it needs deduping.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Potentially there could be a way to reach into the nth item inside the buffer of the stream to get the Chunk responsible for that and then reach into it.

expect(serializedContent.length).toBeGreaterThan(400);

const result = await ReactServerDOMClient.createFromReadableStream(
stream2,
{
ssrManifest: {
moduleMap: null,
moduleLoading: null,
},
},
);

const items = [];
const iterator = result.iterable[Symbol.asyncIterator]();
let entry;
while (!(entry = await iterator.next()).done) {
items.push(entry.value);
}

// Should still match the result when parsed
expect(items.length).toBe(30);
// TODO: These should be the same
// expect(items[5]).toBe(items[10]); // two random items are the same instance
expect(items[5]).toEqual(items[10]);
});

it('warns if passing a this argument to bind() of a server reference', async () => {
const ServerModule = serverExports({
greet: function () {},
Expand Down
Loading