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.

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

Assign resolved outlined props to element object (and not only tuple) #30528

Merged
merged 3 commits into from
Jul 30, 2024
Merged

Assign resolved outlined props to element object (and not only tuple) #30528

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e26781e
Failing test for deduped props of elements in fragments
eps1lon Jul 30, 2024
1e4159e
Assign resolved outlined props to element object (and not only tuple)
unstubbable Jul 30, 2024
1d38832
Add another test for resolving of deduped props
unstubbable Jul 30, 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
14 changes: 14 additions & 0 deletions packages/react-client/src/ReactFlightClient.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -902,6 +902,20 @@ function waitForReference<T>(
handler.value = parentObject[key];
}

// 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 (
parentObject[0] === REACT_ELEMENT_TYPE &&
key === '3' &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the first time we're doing this kind of check? Maybe we should put these indices into variables to make it clearer what we're referencing here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are at least two more cases:

react/packages/react-client/src/ReactFlightClient.js

Line 901 in 927ba57

if (key === '' && handler.value === null) {

react/packages/react-client/src/ReactFlightClient.js

Line 1138 in 927ba57

if (initializingHandler !== null && key === '0') {

Also deferring the answer to @sebmarkbage.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think in this case, this problem is not limited to this particular slot though. It can happen to any key. Like the "type" and the "owner" (in DEV only) too.

So I think we probably should handle every key that an element can have and map it to its property name.

typeof handler.value === 'object' &&
handler.value !== null &&
handler.value.$$typeof === REACT_ELEMENT_TYPE &&
handler.value.props === null
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are there cases where the props are already resolved i.e. handler.value.props !== null?

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'm not sure. At least there's no test failing without the check. But since we don't have 100% test coverage in the flight client, I wanted to be on the safe side.

Copy link
Collaborator

Choose a reason for hiding this comment

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

When we do this at the root it's actually a feature because we don't know that the empty '' key means that it's at the root. The null check implies that we're at the root because we couldn't be waiting for anything else until the root is done.

In this case, this check is kind of unnecessary. If you trust that we do the rest of the logic correct so you don't need it, and if we don't do it right it'll probably break anyway.

So I think just remove this since you need to be able to do this for every other key anyway (key, props, owner, stack...). It's just unnecessary to check all of them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we put the placeholder that we return from waitForReference into a dedicated variable to make it clear what we're referencing here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Deferring to @sebmarkbage here. If we do, we should probably consider the null return value in getOutlinedModel as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's tricky because the placeholder can effect on the hidden class. E.g. undefined is not good for this reason (but also it deletes the property in JSON.parse). Symbols could maybe have similar optimizations to small numbers so feels sketchy too. Most likely this will end up being an object or object-like like string. Maybe an empty object but then that might imply that the next object should have same hidden class.

However, as I mentioned above we also don't need this in this case so I think null is fine.

) {
handler.value.props = parentObject[key];
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I knew about this issue but I was basically ignoring it because I was expecting us to always dedupe parent JSX object itself instead but this is a case where that doesn't happen. I wonder if there's more with keys and stuff.

handler.deps--;

if (handler.deps === 0) {
Expand Down
98 changes: 98 additions & 0 deletions packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,104 @@ describe('ReactFlightDOMBrowser', () => {
expect(container.innerHTML).toBe('{"foo":1}{"foo":1}');
});

it('should handle deduped props of re-used elements in fragments (same-chunk reference)', async () => {
let resolveFooClientComponentChunk;

const FooClient = clientExports(
function Foo({children, item}) {
return children;
},
'1',
'/foo.js',
new Promise(resolve => (resolveFooClientComponentChunk = resolve)),
);

const shared = <div />;

function Server() {
return (
<FooClient track={shared}>
<>{shared}</>
</FooClient>
);
}

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

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

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

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

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

await act(() => {
resolveFooClientComponentChunk();
});

expect(container.innerHTML).toBe('<div></div>');
});

it('should handle deduped props of re-used elements in server components (cross-chunk reference)', async () => {
let resolveFooClientComponentChunk;

function PassthroughServerComponent({children}) {
return children;
}

const FooClient = clientExports(
function Foo({children, item}) {
return children;
},
'1',
'/foo.js',
new Promise(resolve => (resolveFooClientComponentChunk = resolve)),
);

const shared = <div />;

function Server() {
return (
<FooClient track={shared}>
<PassthroughServerComponent>{shared}</PassthroughServerComponent>
</FooClient>
);
}

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

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

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

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

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

await act(() => {
resolveFooClientComponentChunk();
});

expect(container.innerHTML).toBe('<div></div>');
});

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

Expand Down
Loading