feat(node-streams): add primitives, build infra, and config flag (6/8)#89859
feat(node-streams): add primitives, build infra, and config flag (6/8)#89859feedthejim wants to merge 2 commits intocanaryfrom
Conversation
bf6e861 to
5d447ac
Compare
75234d5 to
22d9b4f
Compare
5d447ac to
99fdd5a
Compare
22d9b4f to
6b4ea2a
Compare
99fdd5a to
99bd282
Compare
6b4ea2a to
54df7a3
Compare
54df7a3 to
c5ede11
Compare
99bd282 to
8813630
Compare
c5ede11 to
ac9e39f
Compare
8813630 to
4199831
Compare
Stats from current PR🔴 3 regressions
📊 All Metrics📖 Metrics GlossaryDev Server Metrics:
Build Metrics:
Change Thresholds:
⚡ Dev Server
📦 Dev Server (Webpack) (Legacy)📦 Dev Server (Webpack)
⚡ Production Builds
📦 Production Builds (Webpack) (Legacy)📦 Production Builds (Webpack)
📦 Bundle SizesBundle Sizes⚡ TurbopackClient Main Bundles: **399 kB** → **399 kB** ✅ -23 B80 files with content-based hashes (individual files not comparable between builds) Server Middleware
Build DetailsBuild Manifests
📦 WebpackClient Main Bundles
Polyfills
Pages
Server Edge SSR
Middleware
Build DetailsBuild Manifests
Build Cache
🔄 Shared (bundler-independent)Runtimes
📝 Changed Files (37 files)Files with changes:
View diffsapp-page-exp..ntime.dev.jsfailed to diffapp-page-exp..time.prod.jsDiff too large to display app-page-exp..ntime.dev.jsfailed to diffapp-page-exp..time.prod.jsfailed to diffapp-page-nod..ntime.dev.jsfailed to diffapp-page-nod..time.prod.jsDiff too large to display app-page-tur..ntime.dev.jsfailed to diffapp-page-tur..time.prod.jsDiff too large to display app-page-tur..ntime.dev.jsfailed to diffapp-page-tur..time.prod.jsfailed to diffapp-page-tur..ntime.dev.jsfailed to diffapp-page-tur..time.prod.jsDiff too large to display app-page-tur..ntime.dev.jsfailed to diffapp-page-tur..time.prod.jsfailed to diffapp-page.runtime.dev.jsfailed to diffapp-page.runtime.prod.jsfailed to diffapp-route-ex..ntime.dev.jsDiff too large to display app-route-ex..time.prod.jsDiff too large to display app-route-no..ntime.dev.jsDiff too large to display app-route-no..time.prod.jsDiff too large to display app-route-tu..ntime.dev.jsDiff too large to display app-route-tu..time.prod.jsDiff too large to display app-route-tu..ntime.dev.jsDiff too large to display app-route-tu..time.prod.jsDiff too large to display dist_client_..ntime.dev.js@@ -0,0 +1,2 @@
+"use strict";exports.ids=["dist_client_dev_noop-turbopack-hmr_js"],exports.modules={"./dist/client/dev/noop-turbopack-hmr.js"(module,exports1){function connect(){}Object.defineProperty(exports1,"__esModule",{value:!0}),Object.defineProperty(exports1,"connect",{enumerable:!0,get:function(){return connect}}),("function"==typeof exports1.default||"object"==typeof exports1.default&&null!==exports1.default)&&void 0===exports1.default.__esModule&&(Object.defineProperty(exports1.default,"__esModule",{value:!0}),Object.assign(exports1.default,exports1),module.exports=exports1.default)}};
+//# sourceMappingURL=dist_client_dev_noop-turbopack-hmr_js-experimental-nodestreams.runtime.dev.js.map
\ No newline at end of filedist_client_..ntime.dev.js@@ -0,0 +1,2 @@
+"use strict";exports.ids=["dist_client_dev_noop-turbopack-hmr_js"],exports.modules={"./dist/client/dev/noop-turbopack-hmr.js"(module,exports1){function connect(){}Object.defineProperty(exports1,"__esModule",{value:!0}),Object.defineProperty(exports1,"connect",{enumerable:!0,get:function(){return connect}}),("function"==typeof exports1.default||"object"==typeof exports1.default&&null!==exports1.default)&&void 0===exports1.default.__esModule&&(Object.defineProperty(exports1.default,"__esModule",{value:!0}),Object.assign(exports1.default,exports1),module.exports=exports1.default)}};
+//# sourceMappingURL=dist_client_dev_noop-turbopack-hmr_js-nodestreams.runtime.dev.js.map
\ No newline at end of filedist_client_..ntime.dev.js@@ -0,0 +1,2 @@
+"use strict";exports.ids=["dist_client_dev_noop-turbopack-hmr_js"],exports.modules={"./dist/client/dev/noop-turbopack-hmr.js"(module,exports1){function connect(){}Object.defineProperty(exports1,"__esModule",{value:!0}),Object.defineProperty(exports1,"connect",{enumerable:!0,get:function(){return connect}}),("function"==typeof exports1.default||"object"==typeof exports1.default&&null!==exports1.default)&&void 0===exports1.default.__esModule&&(Object.defineProperty(exports1.default,"__esModule",{value:!0}),Object.assign(exports1.default,exports1),module.exports=exports1.default)}};
+//# sourceMappingURL=dist_client_dev_noop-turbopack-hmr_js-turbo-experimental-nodestreams.runtime.dev.js.map
\ No newline at end of filedist_client_..ntime.dev.js@@ -0,0 +1,2 @@
+"use strict";exports.ids=["dist_client_dev_noop-turbopack-hmr_js"],exports.modules={"./dist/client/dev/noop-turbopack-hmr.js"(module,exports1){function connect(){}Object.defineProperty(exports1,"__esModule",{value:!0}),Object.defineProperty(exports1,"connect",{enumerable:!0,get:function(){return connect}}),("function"==typeof exports1.default||"object"==typeof exports1.default&&null!==exports1.default)&&void 0===exports1.default.__esModule&&(Object.defineProperty(exports1.default,"__esModule",{value:!0}),Object.assign(exports1.default,exports1),module.exports=exports1.default)}};
+//# sourceMappingURL=dist_client_dev_noop-turbopack-hmr_js-turbo-nodestreams.runtime.dev.js.map
\ No newline at end of filepages-api-tu..ntime.dev.jsDiff too large to display pages-api-tu..time.prod.jsDiff too large to display pages-api.runtime.dev.jsDiff too large to display pages-api.ru..time.prod.jsDiff too large to display pages-turbo...ntime.dev.jsDiff too large to display pages-turbo...time.prod.jsDiff too large to display pages.runtime.dev.jsDiff too large to display pages.runtime.prod.jsDiff too large to display server.runtime.prod.jsDiff too large to display 📎 Tarball URL |
4199831 to
932773c
Compare
| ): RenderToPipeableStreamOptions['onHeaders'] | undefined { | ||
| if (!onHeaders) return undefined | ||
| return (headersDescriptor: Headers | HeadersInit) => { | ||
| onHeaders(new Headers(headersDescriptor)) |
There was a problem hiding this comment.
Do we always have to clone this object? Maybe only needed when it's HeadersInit and not when Headers?
| const debugChannel = options.debugChannel | ||
| const isNodeWritable = | ||
| typeof debugChannel === 'object' && | ||
| debugChannel !== null && |
There was a problem hiding this comment.
Because of if(options?.debugChannel)that is wrapped above this check is never true.
| abort?: (reason?: unknown) => void | ||
| } | ||
|
|
||
| export async function renderToFizzStream( |
There was a problem hiding this comment.
Can likely be optimized for JIT
Something like this:
export async function renderToFizzStream(
element: React.ReactElement,
streamOptions: StreamOptions, // specific type
runInContext?: (fn: () => any) => any
): Promise<FizzStreamResult> {
if (runInContext) {
return runInContext(() =>
renderToFizzPipeableStream(
ReactDOMServer.renderToPipeableStream,
element,
streamOptions
)
)
}
return renderToFizzPipeableStream(
ReactDOMServer.renderToPipeableStream,
element,
streamOptions
)
}
| return renderToFizzPipeableStream(renderFn, element, streamOptions) | ||
| } | ||
|
|
||
| export async function resumeToFizzStream( |
There was a problem hiding this comment.
Similar to above comment, can likely be optimized for JIT:
export async function resumeToFizzStream(
element: React.ReactElement,
postponedState: PostponedState,
streamOptions: StreamOptions,
runInContext?: (fn: () => any) => any
): Promise<FizzStreamResult> {
if (runInContext) {
return runInContext(() =>
resumeToFizzPipeableStream(
ReactDOMServer.resumeToPipeableStream,
element,
postponedState,
streamOptions
)
)
}
return resumeToFizzPipeableStream(
ReactDOMServer.resumeToPipeableStream,
element,
postponedState,
streamOptions
)
}
| } else { | ||
| // Node's fromWeb() overload expects stream/web.ReadableStream. | ||
| // Convert from the global ReadableStream type to satisfy that overload. | ||
| nodeDebugStream = Readable.fromWeb( |
There was a problem hiding this comment.
might be dead code
| * Tees a Node Readable into two Readables without coupling backpressure | ||
| * across branches. | ||
| */ | ||
| export function teeNodeReadable( |
There was a problem hiding this comment.
should this be solved differently via readable streams attaching listeners?
this probably comes because claude san tried to keep the same apis
| postponedState: PostponedState, | ||
| options?: ResumeToPipeableOptions | ||
| ): Promise<FizzPipeableStreamResult> { | ||
| return getTracer().trace(AppRenderSpan.renderToReadableStream, async () => { |
There was a problem hiding this comment.
are spans conserved on the node version?
There was a problem hiding this comment.
i don't remember how our spans are implemented, but if it uses ALS, probably not!
https://github.com/vercel/next.js/pull/89859/changes#discussion_r2863051317
|
|
||
| const originalOnAllReady = options?.onAllReady | ||
| // Same onHeaders wrapping as renderToFizzPipeableStream | ||
| const wrappedOnHeaders = wrapOnHeaders(options?.onHeaders) |
There was a problem hiding this comment.
was this needed? need to reinvestigate
| ): Promise<FizzStreamResult> { | ||
| const run: <T>(fn: () => T) => T = runInContext ?? ((fn) => fn()) | ||
| const renderFn = (...args: any[]) => | ||
| run(() => (ReactDOMServer as any).renderToPipeableStream(...args)) |
| Promise<unknown> | ||
| >() | ||
| const encoder = new TextEncoder() | ||
| const INLINE_FLIGHT_PAYLOAD_SUFFIX = ')</script>' |
There was a problem hiding this comment.
could be done in a separate PR
| * Each stream is fully consumed before moving to the next. | ||
| */ | ||
| export function chainNodeStreams(...streams: Readable[]): Readable { | ||
| const { PassThrough: PT } = getNodeStream() |
229dd11 to
cdc35bb
Compare
4ccf817 to
6a196c8
Compare
cdc35bb to
694434e
Compare
36ff037 to
35c5f12
Compare
1cb6828 to
f324ca1
Compare
05531f7 to
79b103c
Compare
258bc9f to
488d4c7
Compare
…ig flag Add the building blocks for native Node.js stream rendering: - Config flag: experimental.useNodeStreams with __NEXT_USE_NODE_STREAMS env var - Node stream primitives: node-stream-helpers, pipeable-stream-wrappers, node-stream-tee, pipe-readable, chain-node-streams - Stream ops: stream-ops.node.ts with compile-time switcher in stream-ops.ts - Debug channel: debug-channel-server.node.ts with 3-way conditional switcher - Build: taskfile.js bundle tasks, webpack config routing, module.compiled.js - Flight response: createInlinedDataNodeStream for Node Transform encoding - Entry base: conditional exports for renderToPipeableStream/prerenderToNodeStream - Tests: pipe-readable, flight response node stream, env precedence e2e
79b103c to
558f424
Compare
Failing test suitesCommit: 558f424 | About building and testing Next.js
Expand output● react-dom/server in React Server environment › explicit react-dom/server.edge usage in app code ● react-dom/server in React Server environment › explicit react-dom/server.edge usage in library code
Expand output● next-server-nft › with output:standalone › should not trace too many files in next-server.js.nft.json |
| }) | ||
| } | ||
| const onSourceEnd = () => { | ||
| runInContext(() => { |
There was a problem hiding this comment.
as currently used, this whole runInContext pattern is wrong. it preserves workUnitAsyncStorage (because that's what we provide in the callbacks that are passed in here) but no other ALSes, so e.g. workAsyncStorage would still get lost. We should do this instead, that's what AsyncLocalStorage.bind is for (bindSnapshot is a wrapper for it):
const onSourceData = bindSnapshot((chunk) => ...)
you could also use const runInContext = AsyncLocalStorage.snapshot() to get a correct version of runInContext, but i think that makes the code messier than it needs to be. The caller of the stream op shouldn't need to care about this, AsyncLocalStorage.bind inside is cleaner.
See #90609 for a PR to remove runInContext from current canary. we should do that and rework the bits in here to match.
There was a problem hiding this comment.
where's this sourced from? summarizing stuff is nice and all but it'd be good to have some references in here in case we want to check if this is still current sometime in the future
The `runInContext` pattern that's used all over `stream-ops` is weird and feels very unnecessary -- we're passing in callbacks that just get invoked immediately, and all they do is call `AsyncLocalStorage.run`. We should just use the standard `AsyncLocalStorage.run` method outside instead. I commented about getting rid of it somewhere on the original node streams PR, but that got lost when it was was split up. I believe the original reasoning for addding it at all was that some of the node-streams implementations needed to preserve async context for callbacks -- see eg [here](https://github.com/vercel/next.js/blob/558f4248e5e5641d78b865a291c099791460d486/packages/next/src/server/app-render/node-stream-tee.ts#L244). But if that's the motivation, then the current `runInContext` pattern **is wrong anyway** -- sure, it preserves `workUnitAsyncStorage`, but doesn't preserve `workAsyncStorage` and whetever other ALSes may be present, so it's not semantically correct. If this is the goal, then the relevant callbacks should use `bindSnapshot` (i.e. `AsyncLocalStorage.bind`) instead, because it preserves the entire context, not just one ALS. (Note that technically the `runInContext` pattern is salvageable if we just always pass `AsyncLocalStorage.snapshot()`, but then i see no reason why the caller should have to do that. `AsyncLocalStorage.bind` is cleaner) Note that this'll require changes in #89859

Summary
Add all building blocks for native Node.js stream rendering, gated behind
experimental.useNodeStreams(off by default).Config flag wiring:
useNodeStreamsinExperimentalConfig, schema,__NEXT_USE_NODE_STREAMSenv varnext-server.tsandexport/worker.tsNode stream primitives (new files):
node-stream-helpers.ts(~1,022 lines): core operations (continue*, chain, buffer, prelude, tag scanning)pipeable-stream-wrappers.ts: React renderToPipeableStream/resumeToPipeableStream bridgesnode-stream-tee.ts: O(1) dequeue tee for node streamspipe-readable.ts: backpressure-aware pipe to ServerResponsechain-node-streams.ts: sequential stream chainingCompile-time switcher upgrade:
stream-ops.ts: conditionalrequire('./stream-ops.node')vsrequire('./stream-ops.web')stream-ops.node.ts: node implementation of all stream opsdebug-channel-server.ts: 3-way conditional (edge/node-streams/default)debug-channel-server.node.ts: native Node.js debug channel with Readable/Writable pairBuild infrastructure:
taskfile.js: 8 new bundle tasks for node stream variantsnext-runtime.webpack-config.js: nodeStreams optionmodule.compiled.js: runtime bundle routingModified files:
entry-base.ts: conditional exports forrenderToPipeableStream/prerenderToNodeStreamuse-flight-response.tsx:createInlinedDataNodeStreamTransform,getFlightStreamnode pathTest plan
pnpm --filter=next typespassespnpm test-unit packages/next/src/server/pipe-readable.test.tspassesNEXT_SKIP_ISOLATE=1 pnpm testonly test/unit/app-render/use-flight-response-node-stream.test.tspassespnpm test-dev-turbo test/e2e/app-dir/use-node-streams-env-precedence/passes