Skip to content

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

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

Merged
merged 3 commits into from
Jul 30, 2024
Merged

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

Changes from 1 commit
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
Prev Previous commit
Next Next commit
Assign resolved outlined props to element object (and not only tuple) 
When an element is used multiple times as shown in #30526, its props
might be deduped. When resolving the reference to the deduped props, we
were only updating the React element tuple, which at this point has
already been parsed into a React element object, using `null` as
placeholder props. Therefore, updating the element tuple doesn't help
much, and we need to make sure that the element object's props are
updated as well.

This is a similar fix as #28669, see the code lines above, and thus
feels similarly hacky. Maybe there's a better way to fix this? @eps1lon
was mentioning offline that solving [this
TODO](https://github.com/facebook/react/blob/33e54fa252b9dbe7553ef42a2287c3dbbd4f035d/packages/react-client/src/ReactFlightClient.js#L1327)
would probably fix it properly, since we wouldn't need to deal with
stale tuples then. But that's a way heavier lift.
  • Loading branch information
@unstubbable
unstubbable committed Jul 30, 2024
commit 1e4159e5ac05e2499b80a3c011565e321d4d984c
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

@eps1lon eps1lon Jul 30, 2024

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

@unstubbable unstubbable Jul 30, 2024

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

@sebmarkbage sebmarkbage Jul 30, 2024

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

@eps1lon eps1lon Jul 30, 2024

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

@unstubbable unstubbable Jul 30, 2024

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

@sebmarkbage sebmarkbage Jul 30, 2024

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

@eps1lon eps1lon Jul 30, 2024

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

@unstubbable unstubbable Jul 30, 2024

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

@sebmarkbage sebmarkbage Jul 30, 2024

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

@sebmarkbage sebmarkbage Jul 30, 2024

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