-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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
[Flight] add support for Lazy components in Flight server #24068
Conversation
Lazy components suspend until resolved just like in Fizz. Add tests to confirm Lazy works with Shared Components and Client Component references.
case REACT_LAZY_TYPE: | ||
throw new Error( | ||
'React Lazy Components are not yet supported on the server.', | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In testing I could not ever get this case to throw. the value at this point is a React.Element and not a symbol so it ends up throwing in attemptResolveElement
instead. If I am missing a use case that did hit this path that needs to be covered by tests let me know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can now use lazy in the position of React "nodes" (i.e. children). That's actually what Flight itself does on the client.
const element = React.lazy(async () => {
return <Component prop="prop" />;
});
...
<div>{element}</div>
That should probably ideally work in Server Components too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also technically, you could pass just the symbol. <Component foo={lazyComponent.$$typeof} />
Regardless, if we see it, it's probably a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add support for it, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should we do if someone does this
const ElementDisguisedAsAComponent = React.lazy(async () => {
return <Component prop={"original"} />;
});
...
<div><ElementDisguisedAsAComponent prop={"new"}</div>
feels like that should error in Dev and in prod we should render the actual element throwing away the "new" prop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works the way I said it should in prod already and there is no conditional, we could add the conditional in dev only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens in the client when you do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the opposite is supported
const componentDisguisedAsElement = React.lazy(async () => {
return Component;
});
...
<div>{componentDisguisedAsElement}</div>
it will render like you did i.e. empty props. I'm thinking this should also error in dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The componentDisguisedAsElement case seems like it should hit this case?
Regardless, it should behave as if there was no lazy around it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a good hueristic, I've updated tests and behavior to have these match. It'll be an error to use lazy elements as Components and vice versa just like without lazy
Comparing: 72a933d...a4329f4 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
React.Lazy can now return an element instead of a Component. This commit implements support for Lazy elements when server rendering.
// this can happen when a lazy component resolves to an element instead of | ||
// a Component. | ||
return attemptResolveElement(type.type, type.key, type.ref, type.props); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the solution is to just remove this and then let it fall through to the error case. You could also put custom error messages at the bottom of this case if you want to explain in plain words to the user what's wrong.
Semantically this is not right since it's dropping key, ref and props from the parent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, the need for this went away when I added lazy resolution to resolveModelToJSON
adding lazying initialization toResolveModelToJson means we use attemptResolveElement's full logic on whatever the resolved type ends up being. This better aligns handling of misued Lazy types like a lazy element being used as a Component or a lazy Component being used as an element.
Summary: This sync includes the following changes: - **[3f8990898](facebook/react@3f8990898 )**: Fix test-build-devtools if build was generated by build-for-devtools ([#24088](facebook/react#24088)) //<Sebastian Silbermann>// - **[577f2de46](facebook/react@577f2de46 )**: enableCacheElement flag ([#24131](facebook/react#24131)) //<David McCabe>// - **[2e0d86d22](facebook/react@2e0d86d22 )**: Allow updating dehydrated root at lower priority without forcing client render ([#24082](facebook/react#24082)) //<Andrew Clark>// - **[dbe9e732a](facebook/react@dbe9e732a )**: Avoid conditions where control flow is sufficient ([#24126](facebook/react#24126)) //<Sebastian Markbåge>// - **[b075f9742](facebook/react@b075f9742 )**: Fix dispatch config type for skipBubbling ([#24109](facebook/react#24109)) //<Luna>// - **[ef23a9ee8](facebook/react@ef23a9ee8 )**: Flag for text hydration mismatch ([#24107](facebook/react#24107)) //<salazarm>// - **[0412f0c1a](facebook/react@0412f0c1a )**: add offscreen state node ([#24026](facebook/react#24026)) //<Luna Ruan>// - **[43eb28339](facebook/react@43eb28339 )**: Add skipBubbling property to dispatch config ([#23366](facebook/react#23366)) //<Luna>// - **[832e2987e](facebook/react@832e2987e )**: Revert accdientally merged PR ([#24081](facebook/react#24081)) //<Andrew Clark>// - **[02b65fd8c](facebook/react@02b65fd8c )**: Allow updates at lower pri without forcing client render //<Andrew Clark>// - **[83b941a51](facebook/react@83b941a51 )**: Add isRootDehydrated function //<Andrew Clark>// - **[c8e4789e2](facebook/react@c8e4789e2 )**: Pass children to hydration root constructor //<Andrew Clark>// - **[581f0c42e](facebook/react@581f0c42e )**: [Flight] add support for Lazy components in Flight server ([#24068](facebook/react#24068)) //<Josh Story>// - **[72a933d28](facebook/react@72a933d28 )**: Gate legacy hidden ([#24047](facebook/react#24047)) //<Sebastian Markbåge>// - **[b9de50d2f](facebook/react@b9de50d2f )**: Update test to reset modules instead of using private state ([#24055](facebook/react#24055)) //<Sebastian Markbåge>// - **[c91892ec3](facebook/react@c91892ec3 )**: [Fizz] Don't flush empty segments ([#24054](facebook/react#24054)) //<Sebastian Markbåge>// - **[d5f1b067c](facebook/react@d5f1b067c )**: [ServerContext] Flight support for ServerContext ([#23244](facebook/react#23244)) //<salazarm>// - **[6edd55a3f](facebook/react@6edd55a3f )**: Gate unstable_expectedLoadTime on enableCPUSuspense ([#24038](facebook/react#24038)) //<Sebastian Markbåge>// - **[57799b912](facebook/react@57799b912 )**: Add more feature flag checks ([#24037](facebook/react#24037)) //<Sebastian Markbåge>// - **[e09518e5b](facebook/react@e09518e5b )**: [Fizz] write chunks to a buffer with no re-use ([#24034](facebook/react#24034)) //<Josh Story>// - **[14c2be8da](facebook/react@14c2be8da )**: Rename Node SSR Callbacks to onShellReady/onAllReady and Other Fixes ([#24030](facebook/react#24030)) //<Sebastian Markbåge>// - **[cb1e7b1c6](facebook/react@cb1e7b1c6 )**: Move onCompleteAll to .allReady Promise ([#24025](facebook/react#24025)) //<Sebastian Markbåge>// - **[566285761](facebook/react@566285761 )**: [Fizz] Export debug function for FB ([#24024](facebook/react#24024)) //<salazarm>// - **[05c283c3c](facebook/react@05c283c3c )**: Fabric HostComponent as EventEmitter: support add/removeEventListener (unstable only) ([#23386](facebook/react#23386)) //<Joshua Gross>// - **[08644348b](facebook/react@08644348b )**: Added unit Tests in the ReactART, increasing the code coverage ([#23195](facebook/react#23195)) //<BIKI DAS>// - **[feefe437f](facebook/react@feefe437f )**: Refactor Cache Code ([#23393](facebook/react#23393)) //<Luna Ruan>// Changelog: [General][Changed] - React Native sync for revisions 1780659...1159ff6 jest_e2e[run_all_tests] Reviewed By: lunaleaps Differential Revision: D34928167 fbshipit-source-id: 8c386f2be5871981d217ab9a514892ed88eafcfb
…4068) * [Flight] add support for Lazy components in Flight server Lazy components suspend until resolved just like in Fizz. Add tests to confirm Lazy works with Shared Components and Client Component references. * Support Lazy elements React.Lazy can now return an element instead of a Component. This commit implements support for Lazy elements when server rendering. * add lazy initialization to resolveModelToJson adding lazying initialization toResolveModelToJson means we use attemptResolveElement's full logic on whatever the resolved type ends up being. This better aligns handling of misued Lazy types like a lazy element being used as a Component or a lazy Component being used as an element.
Summary: This sync includes the following changes: - **[3f8990898](facebook/react@3f8990898 )**: Fix test-build-devtools if build was generated by build-for-devtools ([facebook#24088](facebook/react#24088)) //<Sebastian Silbermann>// - **[577f2de46](facebook/react@577f2de46 )**: enableCacheElement flag ([facebook#24131](facebook/react#24131)) //<David McCabe>// - **[2e0d86d22](facebook/react@2e0d86d22 )**: Allow updating dehydrated root at lower priority without forcing client render ([facebook#24082](facebook/react#24082)) //<Andrew Clark>// - **[dbe9e732a](facebook/react@dbe9e732a )**: Avoid conditions where control flow is sufficient ([facebook#24126](facebook/react#24126)) //<Sebastian Markbåge>// - **[b075f9742](facebook/react@b075f9742 )**: Fix dispatch config type for skipBubbling ([facebook#24109](facebook/react#24109)) //<Luna>// - **[ef23a9ee8](facebook/react@ef23a9ee8 )**: Flag for text hydration mismatch ([facebook#24107](facebook/react#24107)) //<salazarm>// - **[0412f0c1a](facebook/react@0412f0c1a )**: add offscreen state node ([facebook#24026](facebook/react#24026)) //<Luna Ruan>// - **[43eb28339](facebook/react@43eb28339 )**: Add skipBubbling property to dispatch config ([facebook#23366](facebook/react#23366)) //<Luna>// - **[832e2987e](facebook/react@832e2987e )**: Revert accdientally merged PR ([facebook#24081](facebook/react#24081)) //<Andrew Clark>// - **[02b65fd8c](facebook/react@02b65fd8c )**: Allow updates at lower pri without forcing client render //<Andrew Clark>// - **[83b941a51](facebook/react@83b941a51 )**: Add isRootDehydrated function //<Andrew Clark>// - **[c8e4789e2](facebook/react@c8e4789e2 )**: Pass children to hydration root constructor //<Andrew Clark>// - **[581f0c42e](facebook/react@581f0c42e )**: [Flight] add support for Lazy components in Flight server ([facebook#24068](facebook/react#24068)) //<Josh Story>// - **[72a933d28](facebook/react@72a933d28 )**: Gate legacy hidden ([facebook#24047](facebook/react#24047)) //<Sebastian Markbåge>// - **[b9de50d2f](facebook/react@b9de50d2f )**: Update test to reset modules instead of using private state ([facebook#24055](facebook/react#24055)) //<Sebastian Markbåge>// - **[c91892ec3](facebook/react@c91892ec3 )**: [Fizz] Don't flush empty segments ([facebook#24054](facebook/react#24054)) //<Sebastian Markbåge>// - **[d5f1b067c](facebook/react@d5f1b067c )**: [ServerContext] Flight support for ServerContext ([facebook#23244](facebook/react#23244)) //<salazarm>// - **[6edd55a3f](facebook/react@6edd55a3f )**: Gate unstable_expectedLoadTime on enableCPUSuspense ([facebook#24038](facebook/react#24038)) //<Sebastian Markbåge>// - **[57799b912](facebook/react@57799b912 )**: Add more feature flag checks ([facebook#24037](facebook/react#24037)) //<Sebastian Markbåge>// - **[e09518e5b](facebook/react@e09518e5b )**: [Fizz] write chunks to a buffer with no re-use ([facebook#24034](facebook/react#24034)) //<Josh Story>// - **[14c2be8da](facebook/react@14c2be8da )**: Rename Node SSR Callbacks to onShellReady/onAllReady and Other Fixes ([facebook#24030](facebook/react#24030)) //<Sebastian Markbåge>// - **[cb1e7b1c6](facebook/react@cb1e7b1c6 )**: Move onCompleteAll to .allReady Promise ([facebook#24025](facebook/react#24025)) //<Sebastian Markbåge>// - **[566285761](facebook/react@566285761 )**: [Fizz] Export debug function for FB ([facebook#24024](facebook/react#24024)) //<salazarm>// - **[05c283c3c](facebook/react@05c283c3c )**: Fabric HostComponent as EventEmitter: support add/removeEventListener (unstable only) ([facebook#23386](facebook/react#23386)) //<Joshua Gross>// - **[08644348b](facebook/react@08644348b )**: Added unit Tests in the ReactART, increasing the code coverage ([facebook#23195](facebook/react#23195)) //<BIKI DAS>// - **[feefe437f](facebook/react@feefe437f )**: Refactor Cache Code ([facebook#23393](facebook/react#23393)) //<Luna Ruan>// Changelog: [General][Changed] - React Native sync for revisions 1780659...1159ff6 jest_e2e[run_all_tests] Reviewed By: lunaleaps Differential Revision: D34928167 fbshipit-source-id: 8c386f2be5871981d217ab9a514892ed88eafcfb
Lazy components suspend until resolved just like in Fizz. Add tests to confirm Lazy works with Shared Components and Client Component references.