Assign resolved outlined props to element object (and not only tuple) #30528
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
When an element is used multiple times as shown in facebook#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 facebook#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.
unstubbable
force-pushed
the
sebbie/flight-fragment-deduping
branch
from
19ce90e
to
1e4159e
Compare
| ) { | ||
| handler.value.props = parentObject[key]; | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
| typeof handler.value === 'object' && | ||
| handler.value !== null && | ||
| handler.value.$$typeof === REACT_ELEMENT_TYPE && | ||
| handler.value.props === null |
There was a problem hiding this comment.
Are there cases where the props are already resolved i.e. handler.value.props !== null?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| typeof handler.value === 'object' && | ||
| handler.value !== null && | ||
| handler.value.$$typeof === REACT_ELEMENT_TYPE && | ||
| handler.value.props === null |
There was a problem hiding this comment.
Should we put the placeholder that we return from waitForReference into a dedicated variable to make it clear what we're referencing here?
There was a problem hiding this comment.
Deferring to @sebmarkbage here. If we do, we should probably consider the null return value in getOutlinedModel as well.
There was a problem hiding this comment.
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.
| // parsed element object (i.e. handler.value). | ||
| if ( | ||
| parentObject[0] === REACT_ELEMENT_TYPE && | ||
| key === '3' && |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
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.
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.
This playwright test is using the Flight Fixture to demonstrate the Flight Reply equivalent of the scenario that was fixed in facebook#30528 for the Flight Client. It's basically an advanced case of what was outlined in facebook#28564, returning a client component from a server action that is used in `useActionState`. In addition, the client component uses another element twice, which leads to the element's props being deduped. Resolving those references needs to be handled specifically, both in the Flight Client (done in facebook#30528), as well as in the temporary references of the Flight Reply Client (and possibly Flight Reply Server?). The test should probably be converted into a unit test, e.g. in `packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js` or `packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMReply-test.js`.
|
Howdy @unstubbable and @sebmarkbage , I believe this change has introduced a regression that leads to this bug: vercel/next.js#69545 Basically an unhandled runtime error when 2 or more distinct SVGR I'm not familiar with this code, could you advice if this seems to be the cause? 🙇 |
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.
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.
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
nullas 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 would probably fix it properly, since we wouldn't need to deal with stale tuples then. But that's a way heavier lift.