-
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] Move around the Server side a bit #17251
Conversation
This will need to use a variant of Fizz to do inline SSR in Flight. However, I don't want to build the whole impl right now but also don't want to exclude the use case yet. So I outsource it to the existing renderer. Ofc, this doesn't work with Suspense atm.
Size changes (experimental)Details of bundled changes.Comparing: fadc971...b36dfdf react-dom
react-server
|
Size changes (stable)Details of bundled changes.Comparing: fadc971...b36dfdf react-dom
react-server
|
await waitForSuspense(() => { | ||
expect(result.model).toEqual({ | ||
html: '<div><span>hello</span><span>world</span></div>', | ||
}); |
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.
Mind asserting we actually eventually got here?
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.
There’s going to be a hundred of these. I’m not going to assert in each one that the testing environment works.
Jest fails it the promise to the test never resolves.
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 mean the promise could resolve without the inner expect actually having run successfully. If there’s a bug in waitForSuspense.
await waitForSuspense(() => { | ||
expect(result.model).toEqual({ | ||
html: '<div><span>hello</span><span>world</span></div>', | ||
}); |
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.
Same
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.
sure
Builds on top of #17234.
Just some clean up before I add more stuff.