Add Next.js runtime support to ReactFizzServer #22350
Conversation
|
Comparing: 56efc65...a290681 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
|
|
I'm curious why Node APIs or Browser APIs are problematic here? (I can think of some reasons why the Browser API are problematic) For Node, can't we leverage the drain event + stream.write to check if the buffer is full? If not, perhaps it's better for React to create its own Stream Interface that people can hook their implementation in. An out of the box solution for streams can be provided. It would mean that no framework-specific code needs to be in React. |
The problem is server runtimes that aren't Node (think Deno or Cloudflare Workers).
I'm not opposed to this, but would want alignment from the React folks first. |
We're not really in the space of defining general purpose streaming APIs. Seems better that hosts that know what the best lowest level protocol they can expose and that they're willing to support for multiple purposes. If there are attempts for standardizing those, then having those discussions seems better with the people involved in those hosts. We are however in the space of supporting multiple different platforms so it's pretty trivial to support multiple if they're big enough. |
| startWriting() { | ||
| startFlowing(request); | ||
| }, | ||
| stopWriting() { |
There was a problem hiding this comment.
I was planning on renaming startWriting so naming this as a pair wouldn't necessarily make sense.
There was a problem hiding this comment.
Not quite sure what you mean. Would a single function to toggle on/off be better? Or otherwise could you elaborate?
| @@ -1899,7 +1899,7 @@ function flushCompletedQueues(request: Request): void { | |||
| } | |||
|
|
|||
| export function startWork(request: Request): void { | |||
| scheduleWork(() => performWork(request)); | |||
| scheduleWork(request.destination, () => performWork(request)); | |||
There was a problem hiding this comment.
While it might make sense to want to schedule based on the destination, the assumption of this API implies a lot more of a global scheduler like postTask. I worry that this might take us down the wrong assumptions.
For sync I/O, it'll also be required that it's an instance (or at least thread) per request so in that world it doesn't matter anyway.
What is the intention/purpose of this?
There was a problem hiding this comment.
Eh, just some flexibility. I can revert it and make it use a global instead, that wouldn't be a problem.
devknoll
changed the title
|
ping @sebmarkbage, what’s needed to unblock this? |
Summary
Support for a Next.js runtime in
ReactFizzServerthat doesn't rely on Node or Browser APIs.How did you test this change?
Implemented in Next.js (https://github.com/azukaru/next.js/pull/695/files) and ran against CI.