Skip to content
Merged
2 changes: 1 addition & 1 deletion packages/vinext/src/entries/app-rsc-entry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2139,7 +2139,7 @@ async function _handleRequest(request, __reqCtx, _mwCtx) {
markDynamicUsage,
method,
middlewareContext: _mwCtx,
params,
params: makeThenableParams(params),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the only place in the generated entry where params wasn't wrapped. makeThenableParams() is already applied to page params (line ~1776), layout params (line ~1903), metadata params, and slot params. Good catch filling the gap.

reportRequestError: _reportRequestError,
request,
revalidateSeconds,
Expand Down
66 changes: 50 additions & 16 deletions packages/vinext/src/shims/dynamic.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,23 @@
"use client";
/**
* next/dynamic shim
*
* SSR-safe dynamic imports. On the server, uses React.lazy + Suspense so that
* renderToReadableStream suspends until the dynamically-imported component is
* available. On the client, also uses React.lazy for code splitting.
*
* Works in RSC, SSR, and client environments:
* - RSC: Uses React.lazy + Suspense (available in React 19.x react-server).
* Falls back to async component pattern if a future React version
* strips lazy from react-server.
* - SSR: React.lazy + Suspense (renderToReadableStream suspends)
* - Client: React.lazy + Suspense (standard code splitting)
*
* Supports:
* - dynamic(() => import('./Component'))
* - dynamic(() => import('./Component'), { loading: () => <Spinner /> })
* - dynamic(() => import('./Component'), { ssr: false })
*/
import React, { lazy, Suspense, type ComponentType, useState, useEffect } from "react";
import React, { type ComponentType } from "react";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean change. Switching from destructured { lazy, Suspense, useState, useEffect } to a single React namespace import and accessing them as React.lazy, React.Suspense, etc. is the right approach. The destructured form would bind undefined at import time in environments where those APIs are absent, with no way to detect it later. The namespace form lets the runtime typeof checks work correctly.


interface DynamicOptions {
loading?: ComponentType<{ error?: Error | null; isLoading?: boolean; pastDelay?: boolean }>;
Expand Down Expand Up @@ -90,7 +96,7 @@ function dynamic<P extends object = object>(
// ssr: false — render nothing on the server, lazy-load on client
if (!ssr) {
if (isServer) {
// On the server, just render the loading state or nothing
// On the server (SSR or RSC), just render the loading state or nothing
const SSRFalse = (_props: P) => {
return LoadingComponent
? React.createElement(LoadingComponent, { isLoading: true, pastDelay: true, error: null })
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor concern: when dynamic(() => import('./Foo'), { ssr: false }) is called in a server component, this renders LoadingComponent via React.createElement. In the RSC environment, React.createElement is available, but if LoadingComponent is a client component (common for spinners with CSS animations), this will work because the RSC plugin handles client references during rendering.

However, if LoadingComponent itself uses hooks like useState/useEffect (which are unavailable in RSC), this would crash. This is an edge case — most loading spinners are pure presentational — and it matches Next.js behavior (Next.js also renders the loading component on the server). Just flagging it for awareness; no action needed.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged — this matches Next.js behavior and the edge case is inherently user error (passing a hook-using component as loading in a server component context). The common case (presentational spinners, static JSX) works correctly. No action taken per your note.

Expand All @@ -101,15 +107,15 @@ function dynamic<P extends object = object>(
}

// Client: use lazy with Suspense
const LazyComponent = lazy(async () => {
const LazyComponent = React.lazy(async () => {
const mod = await loader();
if ("default" in mod) return mod as { default: ComponentType<P> };
return { default: mod as ComponentType<P> };
});

const ClientSSRFalse = (props: P) => {
const [mounted, setMounted] = useState(false);
useEffect(() => setMounted(true), []);
const [mounted, setMounted] = React.useState(false);
React.useEffect(() => setMounted(true), []);

if (!mounted) {
return LoadingComponent
Expand All @@ -120,7 +126,11 @@ function dynamic<P extends object = object>(
const fallback = LoadingComponent
? React.createElement(LoadingComponent, { isLoading: true, pastDelay: true, error: null })
: null;
return React.createElement(Suspense, { fallback }, React.createElement(LazyComponent, props));
return React.createElement(
React.Suspense,
{ fallback },
React.createElement(LazyComponent, props),
);
};

ClientSSRFalse.displayName = "DynamicClientSSRFalse";
Expand All @@ -129,12 +139,32 @@ function dynamic<P extends object = object>(

// SSR-enabled path
if (isServer) {
// Use React.lazy so that renderToReadableStream can suspend until the
// dynamically-imported component is available. The previous eager-load
// pattern relied on flushPreloads() being called before rendering, which
// works for the Pages Router but not the App Router where client modules
// are loaded lazily during RSC stream deserialization (issue #75).
const LazyServer = lazy(async () => {
// Defensive fallback: if a future React version strips React.lazy from the
// react-server condition, fall back to an async component pattern.
// In React 19.x, React.lazy IS available in react-server, so this branch
// does not execute — it exists for forward compatibility only.
if (typeof React.lazy !== "function") {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This guard may not trigger with current React 19.x — React.lazy is exported from the react-server condition entry (react.react-server.production.js exports lazy at line ~396). If React.lazy IS a function in the RSC environment, this branch is dead code and the existing LazyServer path (lines 159-180) handles RSC rendering via React.lazy + React.Suspense.

This isn't necessarily a problem — the guard is defensive and correct for forward compatibility. But it means:

  1. The regression test at /nextjs-compat/dynamic/rsc-dynamic may pass because of the existing LazyServer path, not this new async path.
  2. If you want to verify this path works, you'd need a test that stubs React.lazy to undefined.

Could you confirm whether this branch actually executes in the test? A console.log or debugger would tell you. If it doesn't, consider adding a unit test with a mocked React.lazy = undefined to cover it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two prior reviews both flagged this and it hasn't been addressed: in React 19.x, React.lazy IS a function in the react-server condition, so this guard never triggers. The AsyncServerDynamic path is dead code today.

This is fine as defensive forward-compat code, but the comment should say so explicitly. The current comment states definitively that react-server doesn't have lazy, which is incorrect for the React version this project actually uses.

Suggested change
if (typeof React.lazy !== "function") {
// Defensive fallback: if a future React version strips React.lazy from the
// react-server condition, fall back to an async component pattern.
// In React 19.x, React.lazy IS available in react-server, so this branch
// does not execute — it exists for forward compatibility only.
if (typeof React.lazy !== "function") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments here are now accurate — good job addressing the prior review feedback. The guard, the AsyncServerDynamic implementation, and the cast comment all correctly describe this as forward-compat code.

One nit: the getDynamicErrorBoundary() check at line 180 also handles the RSC case where React.Component is unavailable (it returns null and the error boundary is skipped). This is consistent with the AsyncServerDynamic path's approach of deferring error handling to the nearest error boundary. Worth noting that both paths degrade gracefully in RSC.

const AsyncServerDynamic = async (props: P) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The LoadingComponent option is silently ignored in the RSC async path. This is likely the correct behavior (the RSC stream suspends natively on the async component, and any <Suspense> boundary higher in the tree provides the loading fallback). But it's a behavioral difference from the SSR path where LoadingComponent is rendered as the <Suspense> fallback.

Worth a comment noting this is intentional:

Suggested change
const AsyncServerDynamic = async (props: P) => {
const AsyncServerDynamic = async (props: P) => {
// Note: LoadingComponent is not used here — in the RSC environment,
// async components suspend natively and parent <Suspense> boundaries
// provide loading states.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This async function doesn't have error handling. If loader() rejects, the error propagates uncaught through the RSC renderer. In the SSR path, getDynamicErrorBoundary() wraps the lazy component so loader failures render the LoadingComponent with the error. The RSC path skips this because React.Component (needed for the error boundary class) is also unavailable under react-server. This is acceptable — RSC errors will be caught by the nearest error boundary in the component tree — but worth being aware of.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous review flagged that loader rejections in this async path propagate uncaught (no error boundary wrapping). That's acceptable since RSC errors propagate to the nearest error boundary in the component tree. But the LoadingComponent option is silently ignored — if a caller passes { loading: () => <Spinner /> }, they won't see it in RSC. Worth a comment:

Suggested change
const AsyncServerDynamic = async (props: P) => {
const AsyncServerDynamic = async (props: P) => {
// Note: LoadingComponent is not used here — in the RSC environment,
// async components suspend natively and parent <Suspense> boundaries
// provide loading states. Error handling also defers to the nearest
// error boundary in the component tree.

// Note: LoadingComponent is not used here — in the RSC environment,
// async components suspend natively and parent <Suspense> boundaries
// provide loading states. Error handling also defers to the nearest
// error boundary in the component tree.
const mod = await loader();
const Component =
"default" in mod
? (mod as { default: ComponentType<P> }).default
: (mod as ComponentType<P>);
return React.createElement(Component, props);
};
AsyncServerDynamic.displayName = "DynamicAsyncServer";
// Cast is safe: async components are natively supported by the RSC renderer,
// but TypeScript's ComponentType<P> doesn't account for async return types.
return AsyncServerDynamic as unknown as ComponentType<P>;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The as unknown as ComponentType<P> cast is needed because AsyncServerDynamic is an async function (returns Promise<ReactElement>) which doesn't match the synchronous ComponentType<P> signature. This is fine because the RSC renderer natively supports async components — but it's worth a brief comment explaining why the cast is safe, since it looks suspicious at first glance.

Suggested change
return AsyncServerDynamic as unknown as ComponentType<P>;
// Cast is safe: async components are natively supported by the RSC renderer,
// but TypeScript's ComponentType<P> doesn't account for async return types.
return AsyncServerDynamic as unknown as ComponentType<P>;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous review suggested a comment on this cast. Agree it's worth it — the as unknown as ComponentType<P> is suspicious without context:

Suggested change
return AsyncServerDynamic as unknown as ComponentType<P>;
// Cast is safe: async components are natively supported by the RSC renderer,
// but TypeScript's ComponentType<P> doesn't account for async return types.
return AsyncServerDynamic as unknown as ComponentType<P>;

}

// SSR path: Use React.lazy so that renderToReadableStream can suspend
// until the dynamically-imported component is available.
const LazyServer = React.lazy(async () => {
const mod = await loader();
if ("default" in mod) return mod as { default: ComponentType<P> };
return { default: mod as ComponentType<P> };
Expand All @@ -151,15 +181,15 @@ function dynamic<P extends object = object>(
const content = ErrorBoundary
? React.createElement(ErrorBoundary, { fallback: LoadingComponent }, lazyElement)
: lazyElement;
return React.createElement(Suspense, { fallback }, content);
return React.createElement(React.Suspense, { fallback }, content);
};

ServerDynamic.displayName = "DynamicServer";
return ServerDynamic;
}

// Client path: standard React.lazy with Suspense
const LazyComponent = lazy(async () => {
const LazyComponent = React.lazy(async () => {
const mod = await loader();
if ("default" in mod) return mod as { default: ComponentType<P> };
return { default: mod as ComponentType<P> };
Expand All @@ -169,7 +199,11 @@ function dynamic<P extends object = object>(
const fallback = LoadingComponent
? React.createElement(LoadingComponent, { isLoading: true, pastDelay: true, error: null })
: null;
return React.createElement(Suspense, { fallback }, React.createElement(LazyComponent, props));
return React.createElement(
React.Suspense,
{ fallback },
React.createElement(LazyComponent, props),
);
};

ClientDynamic.displayName = "DynamicClient";
Expand Down
12 changes: 9 additions & 3 deletions packages/vinext/src/shims/layout-segment-context.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,15 @@
/**
* Layout segment context provider.
*
* This is a "use client" module because it needs React's createContext
* and useContext, which are NOT available in the react-server condition.
* The RSC entry renders this as a client component boundary.
* Must be "use client" so that Vite's RSC bundler renders this component in
* the SSR/browser environment where React.createContext is available. The RSC
* entry imports and renders LayoutSegmentProvider directly, but because of the
* "use client" boundary the actual execution happens on the SSR/client side
* where the context can be created and consumed by useSelectedLayoutSegment(s).
*
* Without "use client", this runs in the RSC environment where
* React.createContext is undefined, getLayoutSegmentContext() returns null,
* the provider becomes a no-op, and useSelectedLayoutSegments always returns [].
*
* The context is shared with navigation.ts via getLayoutSegmentContext()
* to avoid creating separate contexts in different modules.
Comment on lines 5 to 17
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updated comment is accurate and well-written. However, the PR description says "use client" was removed from this file, when in reality it was kept (correctly). The description's Problem section (#2), Solution section, Changes table, and Risk section all claim removal. Please update the PR description to match the actual change — which is a comment rewrite explaining why the directive is necessary.

Expand Down
12 changes: 6 additions & 6 deletions tests/__snapshots__/entry-templates.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -1839,7 +1839,7 @@ async function _handleRequest(request, __reqCtx, _mwCtx) {
markDynamicUsage,
method,
middlewareContext: _mwCtx,
params,
params: makeThenableParams(params),
reportRequestError: _reportRequestError,
request,
revalidateSeconds,
Expand Down Expand Up @@ -4036,7 +4036,7 @@ async function _handleRequest(request, __reqCtx, _mwCtx) {
markDynamicUsage,
method,
middlewareContext: _mwCtx,
params,
params: makeThenableParams(params),
reportRequestError: _reportRequestError,
request,
revalidateSeconds,
Expand Down Expand Up @@ -6239,7 +6239,7 @@ async function _handleRequest(request, __reqCtx, _mwCtx) {
markDynamicUsage,
method,
middlewareContext: _mwCtx,
params,
params: makeThenableParams(params),
reportRequestError: _reportRequestError,
request,
revalidateSeconds,
Expand Down Expand Up @@ -8466,7 +8466,7 @@ async function _handleRequest(request, __reqCtx, _mwCtx) {
markDynamicUsage,
method,
middlewareContext: _mwCtx,
params,
params: makeThenableParams(params),
reportRequestError: _reportRequestError,
request,
revalidateSeconds,
Expand Down Expand Up @@ -10667,7 +10667,7 @@ async function _handleRequest(request, __reqCtx, _mwCtx) {
markDynamicUsage,
method,
middlewareContext: _mwCtx,
params,
params: makeThenableParams(params),
reportRequestError: _reportRequestError,
request,
revalidateSeconds,
Expand Down Expand Up @@ -13221,7 +13221,7 @@ async function _handleRequest(request, __reqCtx, _mwCtx) {
markDynamicUsage,
method,
middlewareContext: _mwCtx,
params,
params: makeThenableParams(params),
reportRequestError: _reportRequestError,
request,
revalidateSeconds,
Expand Down
72 changes: 72 additions & 0 deletions tests/dynamic.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -129,3 +129,75 @@ describe("flushPreloads", () => {
expect(result).toEqual([]);
});
});

// ─── RSC async component path ────────────────────────────────────────────
//
// React 19.x exports React.lazy from the react-server condition, so the
// `typeof React.lazy !== "function"` guard does NOT trigger in current
// React. The AsyncServerDynamic path is defensive forward-compatibility
// code for hypothetical future React versions that strip lazy from RSC.
//
// We verify it here by temporarily stubbing React.lazy to undefined,
// simulating the react-server environment of older or stripped React builds.

describe("next/dynamic RSC async component path (React.lazy unavailable)", () => {
it("returns an async component (DynamicAsyncServer) when React.lazy is not a function", () => {
const originalLazy = React.lazy;
try {
// @ts-expect-error — simulating react-server condition where lazy is absent
React.lazy = undefined;

const DynamicRsc = dynamic(() => Promise.resolve({ default: Hello }));
expect(DynamicRsc.displayName).toBe("DynamicAsyncServer");
} finally {
React.lazy = originalLazy;
}
});

it("async component resolves and renders the dynamically loaded component", async () => {
const originalLazy = React.lazy;
try {
// @ts-expect-error — simulating react-server condition where lazy is absent
React.lazy = undefined;

const DynamicRsc = dynamic(() => Promise.resolve({ default: Hello }));
// The returned component is an async function — call it directly as RSC would
const element = await (DynamicRsc as unknown as (props: object) => Promise<unknown>)({});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good test. Calling the async component directly and asserting on the returned element type is the right way to verify this path, since RSC renderers invoke async components the same way.

// Should return a React element rendered from Hello
expect(element).toBeTruthy();
expect((element as React.ReactElement).type).toBe(Hello);
} finally {
React.lazy = originalLazy;
}
});

it("async component handles modules exporting bare component (no default)", async () => {
const originalLazy = React.lazy;
try {
// @ts-expect-error — simulating react-server condition where lazy is absent
React.lazy = undefined;

const DynamicRsc = dynamic(() => Promise.resolve(Hello as any));
const element = await (DynamicRsc as unknown as (props: object) => Promise<unknown>)({});
expect((element as React.ReactElement).type).toBe(Hello);
} finally {
React.lazy = originalLazy;
}
});

it("async component ignores LoadingComponent (defers to parent Suspense boundary)", () => {
const originalLazy = React.lazy;
try {
// @ts-expect-error — simulating react-server condition where lazy is absent
React.lazy = undefined;

// LoadingComponent is passed but should be silently ignored in RSC path
const DynamicRsc = dynamic(() => Promise.resolve({ default: Hello }), {
loading: LoadingSpinner,
});
expect(DynamicRsc.displayName).toBe("DynamicAsyncServer");
} finally {
React.lazy = originalLazy;
}
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// No "use client" — this is a pure React Server Component.
// Regression test for: https://github.com/cloudflare/vinext/pull/466
//
// In the RSC environment, React.lazy may not be available in future React
// versions (the react-server condition could strip it). dynamic() has a
// defensive fallback to an async component pattern for that scenario.
// In React 19.x, React.lazy IS available in react-server, so this uses
// the standard LazyServer + Suspense path.
import dynamic from "next/dynamic";

export const NextDynamicRscComponent = dynamic(() => import("../text-dynamic-rsc"));
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// No "use client" — this entire page is a React Server Component tree.
// Regression test for: https://github.com/cloudflare/vinext/pull/466
//
// Verifies that dynamic() works in a pure RSC context. Currently React.lazy
// is available in react-server, so the standard lazy path handles this.
// The async fallback path (for future React versions that strip lazy from
// react-server) is tested in tests/dynamic.test.ts via React.lazy stubbing.
import { NextDynamicRscComponent } from "../dynamic-imports/dynamic-rsc";

export default function RscDynamicPage() {
return (
<div id="rsc-dynamic-content">
<NextDynamicRscComponent />
</div>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// No "use client" — this is a pure React Server Component
export default function DynamicRsc() {
return <p id="css-text-dynamic-rsc">next-dynamic dynamic on rsc</p>;
}
24 changes: 22 additions & 2 deletions tests/nextjs-compat/app-routes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,28 @@ describe("Next.js compat: app-routes", () => {
}
});

// ── Catch-all dynamic params (Next.js 15 async params) ──────
// Regression test for: https://github.com/cloudflare/vinext/pull/466
// Route handlers must support `await params` (Promise<{ ... }> pattern).
// Fixture: /api/catch-all/[...slugs]/route.ts uses `await params`
//
// Next.js: 'provides params to routes with dynamic parameters'
// Source: https://github.com/vercel/next.js/blob/canary/test/e2e/app-dir/app-routes/app-custom-routes.test.ts#L84-L92

it("catch-all route handler supports await params (Next.js 15 async params)", async () => {
const res = await fetch(`${baseUrl}/api/catch-all/a/b/c`);
expect(res.status).toBe(200);
const data = await res.json();
expect(data.slugs).toEqual(["a", "b", "c"]);
});

it("catch-all route handler with hyphenated segments", async () => {
const res = await fetch(`${baseUrl}/api/catch-all/foo-bar/baz-qux`);
expect(res.status).toBe(200);
const data = await res.json();
expect(data.slugs).toEqual(["foo-bar", "baz-qux"]);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch removing this from the "documented skips" list now that the fixture exists.

// ── Documented skips ─────────────────────────────────────────
//
// N/A: 'statically generates correctly with no dynamic usage'
Expand Down Expand Up @@ -379,8 +401,6 @@ describe("Next.js compat: app-routes", () => {
// N/A: 'no response returned' — Tests console error inspection
//
// N/A: 'permanentRedirect' — Would need fixture, minor variant of redirect
//
// N/A: 'catch-all routes' — Would need fixture with [...slug] route handler

// ── ISR caching (dev mode) ─────────────────────────────────
// In dev mode, ISR caching is disabled. Route handlers should NOT emit
Expand Down
16 changes: 16 additions & 0 deletions tests/nextjs-compat/dynamic.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,22 @@ describe("Next.js compat: next/dynamic", () => {
expect(html).not.toContain("next-dynamic dynamic no ssr on client");
});

// ── RSC (pure server component) dynamic() ────────────────────

// Regression test for: https://github.com/cloudflare/vinext/pull/466
//
// Verifies that dynamic() works when called from a pure server component.
// In React 19.x, React.lazy IS available in the react-server condition,
// so this exercises the standard LazyServer + Suspense path in RSC.
// The AsyncServerDynamic fallback (for hypothetical future React versions
// that strip lazy) is covered by unit tests in tests/dynamic.test.ts.

it("RSC: dynamic() in a pure server component renders content", async () => {
const { html } = await fetchHtml(baseUrl, "/nextjs-compat/dynamic/rsc-dynamic");
expect(html).toContain("next-dynamic dynamic on rsc");
expect(html).toContain('id="css-text-dynamic-rsc"');
});

// ── Browser-only tests (documented, not ported) ──────────────
//
// SKIP: 'should handle next/dynamic in hydration correctly'
Expand Down
Loading