Adds support for server side suspense and streaming #923
Conversation
packages/babel-plugin/src/index.js
Outdated
| } | ||
|
|
||
| // `lazy()` | ||
| return (path.get('callee').isIdentifier({ name: 'lazy' })) |
There was a problem hiding this comment.
👍
Sounds like a bug. It would be great to get this change as a separate PR
There was a problem hiding this comment.
Need to add some snapshot tests, how do I update snapshots?
There was a problem hiding this comment.
jest -I, or just delete old ones before running the test
|
Great work. But as it changes behavior and extends public API - details has to be discussed. |
|
The code actually does not suspend server-side. Was a bug that made it work. So loads sync on server. |
|
Will redo another way |
|
There, should work as advertised with your tips now :) |
|
Not quite right yet, ssg false seems not to work for some reason. |
|
Sound like it’s a good time to extend loadable testing capability in order to handle new scenarios. That always was a little painful to manual test every change as many important scenarios were not covered, and with the upcoming update that would became too much and too easy to break.
|
|
Was thinking I could try setting up some tests with react-testing-library. Will try to do a pr if that is of interest. |
|
I was thinking about a little more complex webpack based build, but the "complicated" part is why it still not around. |
|
Will make a new issue for testing to move this discussion there #930 |
|
For my education: you added 'flush' variants for LinkTags and ScriptTags, but not for StyleTags. Are they not something that is dynamically discovered during rendering? Also, to confirm: with these modifications, we can continue to use script preloading in head? |
FlushStyleTags should be added. According to reactwg/react-18#114 You should not use preload. |
|
Just needs tests to confirm that the solution works as advertised. |
|
I have seen that, but if so, why are we still collecting link tags? What will those links be for? |
|
I think link tags can be used with preload, if I understand correctly css and js will be loaded in order. |
|
Well, as I understand, we should no longer emit link tags for script preloading, and provide script tags as bootstrap scripts in 'renderToPipeableStream' setup. I am eagerly awaiting a version of Loadable that supports 18 to test these in practice :-). |
|
I think, don't use preload / prefetch In webpack. But load css with preload in link tags so script and css load in order. Need to check out what the browser does. |
|
Ah, OK. So compared to the current 16.8+ code, Loadable.Server will return the same styles, links and script tags, only we would need to keep adding them as they are discovered during streaming (if I understood your 'flush' additions). |
|
Correct :) |
|
https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types So maybe fetchpriority "high" and rel "preload stylesheet" |
|
Loadable needs testing before this can be verified to work. Need to set up e2e testing with cypress or playwright for the example. |
|
You could always do a patch package if you want to use it now, but no guarantees. |
|
Thanks, I will wait for the official version I can just NPM install. |
|
narrator voice: 🐌 xmass break is a season of waiting 🍹 |
|
@fivethreeo reminder that we need 'Elements' variants of 'Tags' for 'flushX' (flushStyleElements, flushLinkElements, flushScriptElements). |
|
@dglozic I am not sure that is needed, because flushing makes little sense when you are rendering instead of streaming html. When rendering you get all html in one go anyway. |
|
Actually yes, sorry, that was silly. When you flush, it goes directly into the stream and you don't really need Elements. Ignore :-). |
|
Been quite sick this week so no time for anything but recuperating. But it is on my radar after I get the e2e testing working since that is needed to verify that this works as it should. So any help on what to test and how to test would help speed it along. I have some ideas ruminating but have not quite figured it out yet. Look at the wip pull for e2e testing. |
|
Sorry to hear that, hope you get well soon! I like @theKashey suggestion to bump the dependencies to React 18 right away, allowing teams to port to React 18 without making any changes to SSR and the use of Loadable. Once we stabilize, we can try out pre-release versions of loadable-components with support for 'flushXYZ'. It would limit the number of variables to watch for. It is not as if React 18 has support for data fetching in Suspense anyway :-) - the API for that is not ready (only 'opinionated frameworks' like Next.js and Remix are experimenting with their own flavors). |
|
So now that dependencies were bumped and I was able to play with React 18.2, I am confused with extending the Writable. Right now, I am not yet doing full streaming: I am using Just for kicks, I replaced const writable = new Writable({
write(chunk, encoding, cb) {
res.write(chunk, encoding, cb);
},
final(cb) {
res.end();
cb();
},
flush() {
if (typeof res.flush === 'function') {
res.flush();
}
},
destroy: (err) => {
res.destroy(err ?? undefined);
}
});
const { pipe } = renderToPipeableStream(rootElement, {
onShellReady: () => {
pipe(writable);
},
onAllReady: () => {
writable.write("");
},
onError: (err) => {
_handleError(err, res, callback);
},
onShellError: (err) => {
_handleError(err, res, callback);
}
});The above code hangs. I expected it to work the same as with |
|
do you have any suspense or errorbouandries in the app? |
|
I do, yes (an ErrorBoundary React component). No Suspense yet. |
|
As I said, the App renders properly when I call |
|
Using express? |
|
Yes, and Node LTS 18 |
|
Try taking out the final handler |
|
I figured it out, but it took actually extending the Writable class: class MyWritable extends Writable {
write(chunk, encoding, cb) {
res.write(chunk, encoding, cb);
}
writev(chunks, encoding, cb) {
res.writev(chunks, encoding, cb);
}
end(chunk, encoding, cb) {
res.end(chunk, encoding, cb);
}
flush() {
if (typeof res.flush === 'function') {
res.flush();
}
}
destroy(err) {
res.destroy(err ?? undefined);
}
}
const writable = new MyWritable(); |
|
My speculation is that React calls other Writable methods that were not implemented in the simple constructor, but are when extending like this. |
|
Ah, I will update the example in the pull. |
|
I don't know if the example itself is affected or not, I just noticed it myself in my own code. I am confused because I have seen other examples on the web similar to yours, yet I could not get it to work (I suspect something about closing the pipe). |
|
Need some jest tests for testing with writeable too. I think a idea is forming on how to test this. Just need to separate all the components into their own app so I can test each kind of loadable component by itself. Then hook into the loadable events I add and do some mocking and rendering with renderToPipeableStream. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Hey @fivethreeo thanks for your work so far on this. |
|
I reckon we can "combine" this PR and #976 (in terms of getting rid of the "old" loadable) to get amazing results. |
Summary
React 18 now supports server side suspense and streaming.
This pull updates the babel plugin to transform
lazy, makes the component suspend server-side and adds two methods toChunkExtractorto incrementally write tags for streaming rendering.Test plan
Run the example app at
https://github.com/fivethreeo/loadable-components/tree/feature/server-side-suspense/examples/streaming-server-side-rendering)