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

Resolve references to deduped owner objects #30549

Merged
merged 5 commits into from
Sep 24, 2024

Conversation

unstubbable
Copy link
Contributor

@unstubbable unstubbable commented Jul 31, 2024

This is a follow-up from #30528 to not only handle props (the critical change), but also the owner and stack of a referenced element.

Handling stacks here is rather academic because the Flight Server currently does not deduplicate owner stacks. And if they are really identical, we should probably just dedupe the whole element. EDIT: Removed from the PR.

Handling owner objects on the other hand is an actual requirement as reported in vercel/next.js#69545. This problem only affects the stable release channel, as the absence of owner stacks allows for the specific kind of shared owner deduping as demonstrated in the unit test.

Copy link

vercel bot commented Jul 31, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 23, 2024 4:41pm

@react-sizebot
Copy link

react-sizebot commented Jul 31, 2024

Comparing: 5d19e1c...9dd6563

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.68 kB 6.68 kB +0.05% 1.82 kB 1.83 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 508.62 kB 508.62 kB = 90.98 kB 90.98 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.69 kB 6.69 kB +0.05% 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 513.56 kB 513.56 kB = 91.71 kB 91.71 kB
facebook-www/ReactDOM-prod.classic.js = 603.01 kB 603.01 kB = 106.69 kB 106.69 kB
facebook-www/ReactDOM-prod.modern.js = 579.24 kB 579.24 kB = 102.79 kB 102.79 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against f4c2115

) {
handler.value.props = parentObject[key];
const element: any = handler.value;
switch (key) {
Copy link
Collaborator

@sebmarkbage sebmarkbage Jul 31, 2024

Choose a reason for hiding this comment

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

What about "key" and "type"? E.g. when they're blocked client references. That should be testable even without deduping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my testing, those were just normal client references, using your example but adapted for clientExports:

"use client";

export const key = "hi";
export const type = "div";
import { key, type as Component } from …

<Component key={key} />

Copy link
Collaborator

Choose a reason for hiding this comment

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

But what if they're not yet resolved? What happens then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a lazy ID that's being resolved, not a reference ID. In other words, the default case of parseModelString is not used, and therefore getOutlinedModel is not called which would call waitForReference. So this code is not used at all. But maybe you're saying we also need to do this dance elsewhere? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, we get this error: Error: Element type is invalid. Received a promise that resolves to: div. Lazy element type must resolve to a class or function.

and the key is the stringified client reference proxy function, it seems: "function() {throw new Error(\"Attempted to call key() from the server but key is on the client. It's not possible to invoke a client function from the server, it can only be rendered as a Component or passed to props of a Client Component.\");}"

packages/react-client/src/ReactFlightClient.js Outdated Show resolved Hide resolved
packages/react-client/src/ReactFlightClient.js Outdated Show resolved Hide resolved
This is a follow-up from facebook#30528 to not only handle props (the critical
change), but also the owner and stack of a referenced element.

Handling stacks here is rather academic because the Flight Server
currently does not deduplicate owner stacks. And if they are really
identical, we should probably just dedupe the whole element.
Comment on lines 686 to 697
// Assigning the path elements to variables in module scope (here simulated
// with the test's function scope), and rendering a second time, prevents
// the owner of the path elements (i.e. Svg1/Svg2) to be deduped. The owner
// of the path in Svg1 is fully inlined. The owner of the owner of the path
// in Svg2 is Server, which is deduped and replaced with a reference to the
// owner of the owner of the path in Svg1. This nested owner is actually
// Server from the previous render pass, which is kinda broken and libaries
// probably shouldn't generate code like this. This reference can only be
// resolved properly if owners are specifically handled when resolving
// outlined models.
Copy link
Contributor Author

@unstubbable unstubbable Sep 23, 2024

Choose a reason for hiding this comment

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

The first render pass yields:

1:{"name":"Server","env":"Server","key":null,"owner":null}
0:D"$1"
3:{"name":"Svg1","env":"Server","key":null,"owner":"$1"}
2:D"$3"
2:["$","svg",null,{"id":"1","children":[["$","path",null,{},"$3"],["$","path",null,{},"$3"]]},"$3"]
5:{"name":"Svg2","env":"Server","key":null,"owner":"$1"}
4:D"$5"
4:["$","svg",null,{"id":"2","children":["$","path",null,{},"$5"]},"$5"]
0:["$2","$4"]

Notice how the owners are fully deduped to their own chunks.

The second render pass yields:

1:{"name":"Server","env":"Server","key":null,"owner":null}
0:D"$1"
3:{"name":"Svg1","env":"Server","key":null,"owner":"$1"}
2:D"$3"
2:["$","svg",null,{"id":"1","children":[["$","path",null,{},{"name":"Svg1","env":"Server","key":null,"owner":{"name":"Server","env":"Server","key":null,"owner":null}}],["$","path",null,{},"$2:props:children:0:_owner"]]},"$3"]
5:{"name":"Svg2","env":"Server","key":null,"owner":"$1"}
4:D"$5"
4:["$","svg",null,{"id":"2","children":["$","path",null,{},{"name":"Svg2","env":"Server","key":null,"owner":"$2:props:children:0:_owner:owner"}]},"$5"]
0:["$2","$4"]

Notice the $2:props:children:0:_owner:owner reference that can now be revolved with the provided fix. Before, the reference was $2:props:children:0:4:owner and could not be resolved, leading to the error: TypeError: Cannot read properties of undefined (reading '$$typeof') in getOutlinedModel.

@unstubbable unstubbable changed the title Handle owner and stack when resolving partially deduped elements Handle owner when resolving partially deduped elements Sep 23, 2024
@unstubbable unstubbable changed the title Handle owner when resolving partially deduped elements Resolve references to deduped owner objects Sep 23, 2024
Previously, the test only covered the change in the react flight server.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants