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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
27 changes: 18 additions & 9 deletions packages/react-client/src/ReactFlightClient.js
Original file line number Diff line number Diff line change
Expand Up @@ -937,26 +937,35 @@ function waitForReference<T>(
}
value = value[path[i]];
}
parentObject[key] = map(response, value);
const mappedValue = map(response, value);
parentObject[key] = mappedValue;

// If this is the root object for a model reference, where `handler.value`
// is a stale `null`, the resolved value can be used directly.
if (key === '' && handler.value === null) {
handler.value = parentObject[key];
handler.value = mappedValue;
}

// If the parent object is an unparsed React element tuple and its outlined
// props have now been resolved, we also need to update the props of the
// parsed element object (i.e. handler.value).
// If the parent object is an unparsed React element tuple, we also need to
// update the props and owner of the parsed element object (i.e.
// handler.value).
if (
parentObject[0] === REACT_ELEMENT_TYPE &&
key === '3' &&
typeof handler.value === 'object' &&
handler.value !== null &&
handler.value.$$typeof === REACT_ELEMENT_TYPE &&
handler.value.props === null
handler.value.$$typeof === REACT_ELEMENT_TYPE
) {
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.\");}"

case '3':
element.props = mappedValue;
break;
case '4':
if (__DEV__) {
element._owner = mappedValue;
}
break;
}
}

handler.deps--;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,119 @@ describe('ReactFlightDOMBrowser', () => {
expect(container.innerHTML).toBe('<div></div>');
});

it('should handle references to deduped owner objects', async () => {
// This is replicating React components as generated by @svgr/webpack:
let path1a: React.ReactNode;
let path1b: React.ReactNode;
let path2: React.ReactNode;

function Svg1() {
return ReactServer.createElement(
'svg',
{id: '1'},
path1a || (path1a = ReactServer.createElement('path', {})),
path1b || (path1b = ReactServer.createElement('path', {})),
);
}

function Svg2() {
return ReactServer.createElement(
'svg',
{id: '2'},
path2 || (path2 = ReactServer.createElement('path', {})),
);
}

function Server() {
return ReactServer.createElement(
ReactServer.Fragment,
{},
ReactServer.createElement(Svg1),
ReactServer.createElement(Svg2),
);
}

let stream = await serverAct(() =>
ReactServerDOMServer.renderToReadableStream(<Server />, webpackMap),
);

function ClientRoot({response}) {
return use(response);
}

let response = ReactServerDOMClient.createFromReadableStream(stream);
const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);

await act(() => {
root.render(<ClientRoot response={response} />);
});

const expectedHtml =
'<svg id="1"><path></path><path></path></svg><svg id="2"><path></path></svg>';

expect(container.innerHTML).toBe(expectedHtml);

// Render a second time:

// 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 libraries
// probably shouldn't generate code like this. This reference can only be
// resolved properly if owners are specifically handled when resolving
// outlined models.

stream = await serverAct(() =>
ReactServerDOMServer.renderToReadableStream(<Server />, webpackMap),
);

response = ReactServerDOMClient.createFromReadableStream(stream);

await act(() => {
root.render(<ClientRoot response={response} />);
});

expect(container.innerHTML).toBe(expectedHtml);

if (__DEV__) {
const resolvedPath1b = await response.value[0].props.children[1]._payload;

expect(resolvedPath1b._owner).toEqual(
expect.objectContaining({
name: 'Svg1',
env: 'Server',
key: null,
owner: expect.objectContaining({
name: 'Server',
env: 'Server',
key: null,
owner: null,
}),
}),
);

const resolvedPath2 = response.value[1].props.children;

expect(resolvedPath2._owner).toEqual(
expect.objectContaining({
name: 'Svg2',
env: 'Server',
key: null,
owner: expect.objectContaining({
name: 'Server',
env: 'Server',
key: null,
owner: null,
}),
}),
);
}
});

it('should progressively reveal server components', async () => {
let reportedErrors = [];

Expand Down
3 changes: 3 additions & 0 deletions packages/react-server/src/ReactFlightServer.js
Original file line number Diff line number Diff line change
Expand Up @@ -2665,6 +2665,9 @@ function renderModelDestructive(
case '3':
propertyName = 'props';
break;
case '4':
propertyName = '_owner';
break;
}
}
writtenObjects.set(value, parentReference + ':' + propertyName);
Expand Down
Loading