[Flight] Build node-webstreams version of bundled webpack server #33456
Conversation
github-actions
bot
added
the
React Core Team
sebmarkbage
force-pushed
the
nodewebstreamswebpack
branch
3 times, most recently
from
b6e1c12 to
8c5b422
Compare
sebmarkbage
force-pushed
the
nodewebstreamswebpack
branch
from
8c5b422 to
c4e4a07
Compare
| installedHook.disable(); | ||
| // We unmount when there's more than two hooks installed. | ||
| // We use two because the build of server.node actually installs two hooks. | ||
| // One in each build. |
There was a problem hiding this comment.
This reveals that we're actually installing two async_hooks when you import server.node. One in the Node build and one in the Node Web Streams build. 😬 Since it's a side-effect of requiring these.
Basically you have to deep link to make it optimized.
| }; | ||
| exports.createServerReference = function (i, c, e, d, f) { | ||
| return w.registerServerReference( | ||
| n.createServerReference(i, c, e, d, f), |
There was a problem hiding this comment.
I missed this in #33442, but c, e, d, f are all ignored for the edge and node builds, aren't they? The original function with all params only seems be exported in the browser build, if I'm not mistaken.
There was a problem hiding this comment.
They are now but I'm not sure they always will so for now just passing along the fully signature in case we change it.
We really should have one of these builds for SSR and a different one for RSC because they have different constraints. Calling a Server Reference from RSC should maybe be ok.
Also, we should really be passing the DEV only findSourceMapURL and functionName through since they can be useful for DEV features.
So we should really accept all args.
| exports.registerServerReference = function (r, i, e) { | ||
| return w.registerServerReference(n.registerServerReference(r, i, e), i, e); | ||
| }; | ||
| exports.createServerReference = function (i, c, e, d, f) { |
There was a problem hiding this comment.
Will this wrapper mess with source mapping? I remember you mentioned in the past that module indirection is ok, but wrapper is not, because React doesn't let you customize how many stack frames to pop.
There was a problem hiding this comment.
That only matters on the server entry point and that one isn't wrapped because they are not using module state but instead attaches meta data on the function. I was hoping to change that to be the shared state model as the client but if we do that we'll just also have to hoist that shared state into a separate internal bundle.
There was a problem hiding this comment.
Actually I'm wrong about this. It doesn't matter for registerServerReference but it does matter for createServerReference.
unstubbable
dismissed
their stale review
The latest question should be clarified first.
Reverts #33457, #33456 and #33442. There are too many issues with wrappers, lazy init, stateful modules, duplicate instantiation of async_hooks and duplication of code. Instead, we'll just do a wrapper polyfill that uses Node Streams internally. I kept the client indirection files that I added for consistency with the server though.
Follow up to #33442. This is the bundled version.
To keep type check passes from exploding and the maintainance of the annoying
paths: []list small, this doesn't add this to flow type checks. We might miss some config but every combination should already be covered by other one passes.I also don't add any jest tests because to test these double export entry points we need conditional importing to cover builds and non-builds which turns out to be difficult for the Flight builds so these aren't covered by any basic build tests.
This approach is what I'm going for, for the other bundlers too.