[Flight] Encode references to existing objects by property path #28996
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
sebmarkbage
force-pushed
the
flightdedupebypath
branch
from
1003fbb
to
d1a82f8
Compare
| @@ -981,8 +992,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); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It is mainly to avoid all of these calls turning into multi-line.
| 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
sebmarkbage
force-pushed
the
flightdedupebypath
branch
2 times, most recently
from
b56b1a4
to
01fe345
Compare
| @@ -543,6 +543,55 @@ describe('ReactFlightDOMEdge', () => { | |||
| expect(await iterator.next()).toEqual({value: undefined, done: true}); | |||
| }); | |||
|
|
|||
| // @gate enableFlightReadableStream | |||
| it('dedupes objects inside async iterables', async () => { | |||
There was a problem hiding this comment.
I'm confused. This is a new test but it asserts that there isn't deduping when the test describes deduping. Can we just gate that it fails until we address the underlying limitation? maybe I am misunderstanding something
There was a problem hiding this comment.
We often just assert on the broken behavior inline with a comment. However, since this entire test is broken I'll name it different.
This doesn't work now.
sebmarkbage
force-pushed
the
flightdedupebypath
branch
from
01fe345
to
ff3a3c1
Compare
Instead of forcing an object to be outlined to be able to refer to it later we can refer to it by the property path inside another parent object. E.g. this encodes such a reference as `'$123:props:children:foo:bar'`. That way we don't have to preemptively outline object and we can dedupe after the first time we've found it. There's no cost on the client if it's not used because we're not storing any additional information preemptively. This works mainly because we only have simple JSON objects from the root reference. Complex objects like Map, FormData etc. are stored as their entries array in the look up and not the complex object. Other complex objects like TypedArrays or imports don't have deeply nested objects in them that can be referenced. This solves the problem that we only dedupe after the third instance. This dedupes at the second instance. It also solves the problem where all nested objects inside deduped instances also are outlined. The property paths can get pretty large. This is why a test on payload size increased. We could potentially outline the reference itself at the first dupe. That way we get a shorter ID to refer to in the third instance. DiffTrain build for [7a78d03](7a78d03)
Uses the same technique as in #28996 to encode references to already emitted objects. This now means that Reply can support cyclic objects too for parity.
Triggered by vercel/next.js#66033 I was suspecting that the bug was introduced with facebook#28996, but I could not make the test succeed on a commit before that PR, so maybe this assumption is wrong.
…ialized' (#29204) Follow up to #29201. If a chunk had listeners attached already (e.g. because `.then` was called on the chunk returned from `createFromReadableStream`), `wakeChunkIfInitialized` would overwrite any listeners added during chunk initialization. This caused cyclic [path references](#28996) within that chunk to never resolve. Fixed by merging the two arrays of listeners.
Instead of forcing an object to be outlined to be able to refer to it later we can refer to it by the property path inside another parent object.
E.g. this encodes such a reference as
'$123:props:children:foo:bar'.That way we don't have to preemptively outline object and we can dedupe after the first time we've found it.
There's no cost on the client if it's not used because we're not storing any additional information preemptively.
This works mainly because we only have simple JSON objects from the root reference. Complex objects like Map, FormData etc. are stored as their entries array in the look up and not the complex object. Other complex objects like TypedArrays or imports don't have deeply nested objects in them that can be referenced.
This solves the problem that we only dedupe after the third instance. This dedupes at the second instance. It also solves the problem where all nested objects inside deduped instances also are outlined.
The property paths can get pretty large. This is why a test on payload size increased. We could potentially outline the reference itself at the first dupe. That way we get a shorter ID to refer to in the third instance.