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.

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

[Flight Reply] Encode Objects Returned to the Client by Reference #29010

Merged
merged 3 commits into from
May 10, 2024

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented May 7, 2024

Stacked on #28997.

We can use the technique of referencing an object by its row + property name path for temporary references - like we do for deduping. That way we don't need to generate an ID for temporary references. Instead, they can just be an opaque marker in the slot and it has the implicit ID of the row + path.

Then we can stash all objects, even the ones that are actually available to read on the server, as temporary references. Without adding anything to the payload since the IDs are implicit. If the same object is returned to the client, it can be referenced by reference instead of serializing it back to the client. This also helps preserve object identity.

We assume that the objects are immutable when they pass the boundary.

I'm not sure if this is worth it but with this mechanism, if you return the FormData payload from a useActionState it doesn't have to be serialized on the way back to the client. This is a common pattern for having access to the last submission as "default value" to the form fields. However you can still control it by replacing it with another object if you want. In MPA mode, the temporary references are not configured and so it needs to be serialized in that case. That's required anyway for hydration purposes.

I'm not sure if people will actually use this in practice though or if FormData will always be destructured into some other object like with a library that turns it into typed data, and back. If so, the object identity is lost.

@sebmarkbage sebmarkbage requested review from gnoff and acdlite May 7, 2024 01:29
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels May 7, 2024
@react-sizebot
Copy link

react-sizebot commented May 7, 2024

The size diff is too large to display in a single comment. The CircleCI job contains an artifact called 'sizebot-message.md' with the full message.

Generated by 🚫 dangerJS against 7c69449

@sebmarkbage sebmarkbage force-pushed the tempobjects branch 2 times, most recently from 5f5f445 to b1045f1 Compare May 9, 2024 23:18
Copy link
Collaborator

@gnoff gnoff left a comment

Choose a reason for hiding this comment

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

I assume you didn't add tests for the behavior of references with : in it because we really should support it with escaping and that will change your assertions but I think there are probably some failure modes for those property names that are going to be really confusing if anyone uses this with colons.

But I spent like 5 minutes thinking about the escaping and it seems non-trivial so I guess I get why it's not in this PR.

Are you going to follow up with that imminently?

Comment on lines +411 to +417
if (parentReference !== undefined) {
// If the parent has a reference, we can refer to this object indirectly
// through the property name inside that parent.
const reference = parentReference + ':' + key;
// Store this object so that the server can refer to it later in responses.
writeTemporaryReference(temporaryReferences, reference, value);
return serializeTemporaryReferenceMarker();
Copy link
Collaborator

Choose a reason for hiding this comment

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

struggling with the constraints here.

When would parentReference be undefined? Presumably never because otherwise you'd fall through the error below which doesn't really explain the case you are hitting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea it would only be in the case of a bug in React I think.

@@ -247,6 +270,7 @@ function resolveModelChunk<T>(chunk: SomeChunk<T>, value: string): void {
const resolvedChunk: ResolvedModelChunk<T> = (chunk: any);
resolvedChunk.status = RESOLVED_MODEL;
resolvedChunk.value = value;
resolvedChunk.reason = id;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wish we could give these properties virtual names. so hard to keep track of what they do in every context you use them

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suppose we could give them helpers like setChunkID(ResolvedModelChunk, number) but I don't find it that big of a deal since it's only in the init case and then when you read it and in each case the variable shows what the alias means when reading the code.

@sebmarkbage
Copy link
Collaborator Author

It's not directly related to this PR but the previous ones. In particular, if you use a : as a property name that's in a cycle - it'll error since the cycle can't be expressed. So that's a known bug.

I'm not going to do it imminently fix it. It's an exercise left for the reader.

This lets us also register objects that are not opaque by reference.
This ensures that objects can only be passed by reference if they're
part of the same pair of renders.

This also ensures that you can avoid this short cut when it's not possible
such as in SSR passes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants