-
Notifications
You must be signed in to change notification settings - Fork 7
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
Use renderToPipeableStream wired up to a ReadableStream to add streaming support #2
Conversation
// We have this in Oxygen and can expose it | ||
globalThis.Buffer = globalThis.Buffer || { |
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.
Alternatively, perhaps we could get react to not depend on it. If not, another approach would be to have hydrogen polyfill 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.
We already have Buffer
in Oxygen, because it was a dependency of some other polyfill, it's just not exposed. So we can expose it and it will work.
@@ -3,8 +3,7 @@ import { DataProvider } from "./data"; | |||
import { ABORT_DELAY, API_DELAY } from "./delays"; | |||
import { | |||
renderToPipeableStream, | |||
renderToReadableStream, | |||
} from "react-dom/server"; | |||
} from "react-dom/cjs/react-dom-server.node.development"; |
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 looks like this is using node specific code. Actually, that looks like that was already the case with renderToPipeableStream.
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 had to provide a very specific path because we set the target
to webworker
, and so yarn imports browser-specific stuff but we need the node implementation.
As for the node-specific code, it only uses setImmediate
and Buffer.from
so far, we have Buffer
and setImmediate
is easy to polyfill (or can be implemented natively in Oxygen). The renderToReadableStream
implementation didn't use any specific node stuff but it sits in react-dom-server.browser.development.js
which suggests it might be eventually using browser-specific APIs. React doesn't target workers as a platform yet, but if we get React streaming to production quality in Oxygen, this can change.
This has since been fixed upstream in React facebook/react#23342 |
This is a first MVP that uses
renderToPipeableStream
wired up to aReadableStream
instance.Unlike directly using
renderToReadableStream
, this still streams (the throbber rotates on the page) but doesn't throw a stream close error and doesn't break HTML markup.