mirror of
https://github.com/cloudflare/vinext.git
synced 2026-09-14 19:04:59 +08:00
vinext@0.1.0
225 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
|
cda19e1c58 |
fix(app-router): preserve recent segment state with Activity BFCache (#1739)
* fix(app-router): preserve recent segment state with Activity BFCache Fresh App Router navigations were reusing the active React tree when moving between dynamic segment values. That leaked client state into a newly entered segment and made the upstream back-forward-cache suite fail before it even reached browser traversal assertions. The root cause was that vinext had history-entry bfcacheId plumbing but no separate segment state-key cache. Next.js treats those identities separately: render state is keyed by segment identity/pathname, while useRouter().bfcacheId identifies a concrete history entry. Add state-key maps for app router slots, render retained entries through React.Activity when cacheComponents is enabled, keep the active one-entry keyed reset path otherwise, and port focused Next.js regression coverage for the three-entry BFCache behavior. * fix(bfcache): store stateKeyMap in retained slot entries and re-provide on render Review feedback requested two changes: 1. Add stateKeyMap to BfcacheSlotEntry so each retained BFCache entry carries the state-key map that belonged to it when active, rather than borrowing the current navigation's map. This prevents nested hidden entries from being rendered with old elements but new pathname-derived state keys — an identity corruption bug that upstream Next.js avoids by rendering each retained entry from its own state. 2. Add a nested dynamic regression test: /nested/[section]/item/[id] with client counters in both the [section] layout and [id] page. The existing flat /page/[n] tests do not exercise nested retained-entry identity, which is exactly where the missing state-key-map bug lives. Changes: - BfcacheSlotEntry gains stateKeyMap?: Readonly<Record<string, string>> - updateBfcacheSlotEntries accepts and stores an optional stateKeyMap argument - BfcacheSlotBoundary stores the current stateKeyMap alongside elements when caching the active entry - Both the Activity visible/hidden path and the non-cacheComponents single-entry path now wrap rendered entries in BfcacheStateKeyMapContext.Provider using the entry's own stateKeyMap (falling back to current map for forward compatibility) - New e2e test verifies nested layout + page counter preservation across segment navigations * refactor(bfcache): extract shared provider stack into BfcacheEntryProviders BfcacheSlotBoundary rendered the BfcacheStateKeyMapContext / ElementsContext / SegmentContext provider stack in two places: the one-entry keyed reset path and the per-entry Activity retention list. The two stacks were identical except for where the React key sat, so the retained-entry bookkeeping and its render shape were tangled together and harder to audit than needed. Extract BfcacheEntryProviders as a pure presentational helper and render it from both paths. The keyed-reset path keeps its remount semantics: the key now sits on the BfcacheEntryProviders element, so a fresh active stateKey still remounts the whole provider subtree. Hook ordering is unchanged. All hooks stay above the early returns in BfcacheSlotBoundary, so the boundary still satisfies the Rules of Hooks across the activeStateKey undefined-to-defined transition. Covered by the slot BFCache entry-state suite in tests/shims.test.ts. * test(bfcache): isolate cacheComponents BFCache coverage in a dedicated fixture The back/forward-cache e2e suite ran against the shared app-basic fixture with cacheComponents: true. app-basic backs the single app-router Playwright server that every app-router spec uses, so enabling cacheComponents globally changed a core DOM invariant for unrelated tests: inactive route trees stay mounted as hidden Activity DOM. The contamination already leaked a :visible workaround into the interception-modal bfcacheId spec, which had nothing to do with Activity. cacheComponents is an opt-in semantic mode, not default app-router behaviour, so it does not belong on the shared baseline fixture. Move the BFCache routes into a dedicated tests/fixtures/app-bfcache fixture with cacheComponents: true, served by a new app-router-bfcache Playwright project on port 4183, and run the back-forward-cache spec there against the project baseURL. The :visible locators stay in that spec, where duplicate hidden DOM is the expected contract. Remove cacheComponents and the BFCache routes from app-basic so the shared fixture models default behaviour again, and revert the :visible workaround in use-router-bfcache-id.spec.ts now that the modal route no longer retains hidden DOM. Fix the nested item page import while moving it: nested/[section]/item/[id]/ page.tsx imported ../counter, but the counter lives two levels up at nested/[section]/counter.tsx, so it must be ../../counter. Validation: PLAYWRIGHT_PROJECT=app-router-bfcache (5 passed) and the full use-router-bfcache-id spec under app-router (11 passed), plus the slot BFCache unit suite (1172 passed) and vp check (formatting + lint + types clean). * ci: run the app-router-bfcache e2e project in the matrix The dedicated app-router-bfcache Playwright project was added to playwright.config.ts but not to the CI E2E matrix, so the back/forward-cache specs never ran in CI. Add it to the matrix project list so the BFCache suite runs on every push. It uses the default chromium browser, so the existing setup and browser-cache steps apply unchanged. * fix(bfcache): keep persisted slots mounted when cacheComponents is off Without cacheComponents there is no Activity retention, yet BfcacheSlotBoundary still keyed the active entry by its segment stateKey. That stateKey tracks the pathname (createBfcacheSegmentIdentity), so any slot whose identity moves with the URL (shared layouts, interception source slots) was remounted on navigation, discarding client state that survives a normal route change. The baseline router reconciles these slots in place. This regressed seven existing app-router e2e tests once cacheComponents was no longer masking it on the shared fixture: interception source-context and modal preservation in advanced.spec.ts, and layout-persistence.spec.ts dynamic-segment layout counter survival. Fresh-entry reset is driven by userland bfcacheId keying in the fixtures, not by remounting the slot subtree, so the keying was pure overreach in the non-cacheComponents path. Render the single active entry unkeyed when cacheComponents is off so the boundary reconciles in place like the baseline. The keyed Activity retention path is unchanged for cacheComponents. Validation: full app-router project 436 passed / 0 failed (the seven regressions now pass); use-router-bfcache-id 11 passed (fresh-push reset intact); app-bfcache 5 passed (Activity retention intact); shims unit suite 1024 passed. * docs(test): point bfcacheId spec comment at the app-router-bfcache project The header noted that Activity-backed preservation is covered in back-forward-cache.spec.ts, but that spec moved to the dedicated app-bfcache fixture under the app-router-bfcache project. Name the project and path so the cross-reference stays accurate. * docs: align cacheComponents comment with unkeyed active-segment behavior * test(bfcache): assert fresh bfcacheId after Activity-restored return * style: collapse locator chains per oxfmt * refactor(bfcache): split slot entry order from snapshots * fix(bfcache): keep non-cacheComponents slot path baseline * fix(bfcache): use string cacheComponents define * test: remove external fetch from revalidate tag fixture * fix(bfcache): harden Activity state key retention * docs(bfcache): document cacheComponents Activity caveats * fix(bfcache): preserve navigation context for fallback SSR Fallback boundary rendering can outlive the original request-scoped navigation context. That made app SSR fail fast with a missing navigation context when loading-boundary digest routes rendered their HTTP fallback shell. The SSR boundary contract still requires a navigation context for real request renders, but boundary renderers now synthesize the same typed shape from the request URL and matched params when request scope has already been cleared. * refactor(bfcache): normalize pathname in state-key map, document invariant callers Moves hydration-pathname normalization into createBfcacheSegmentStateKeyMap so SSR and client always produce byte-identical Activity keys. Documents the requireNavigationContext guaranteeing callers per review feedback. * fix(bfcache): tolerate malformed state-key pathnames * docs(bfcache): document known Next.js divergence and dead-branch contract * chore(deps): update lockfile for vite 0.1.24 * fix(app-router): preserve encoded delimiters in bfcache keys * docs(bfcache): track router-owned retention follow-up * chore: retrigger ci * fix(app-router): preserve redirect bridge fallback |
||
|
|
aab9240e10 |
fix(app-router): hard navigate streamed redirects (#1742)
* fix(app-router): hard navigate streamed redirects * fix(navigation): guard page redirect marker checks * test(app-router): count redirect document loads |
||
|
|
bc31644649 |
fix(app-router): match streaming metadata error responses (#1794)
* fix(app-router): match streaming metadata error responses Vinext currently serializes metadata-originated redirects into a Flight payload for document requests. That response is not an HTML page, so direct browser visits cannot follow the streamed redirect and html-limited bots receive 200 instead of the blocking redirect. The dispatcher also renders HTTP access fallback head metadata from layouts only, so not-found boundary metadata is omitted when generateMetadata() throws notFound(). Use Next.js's split response model: RSC requests keep the Flight redirect digest, streaming-capable document requests receive the refresh meta tag HTML, and html-limited bots fall through to the HTTP redirect. Thread the selected HTTP access boundary module into fallback rendering so boundary metadata is resolved exactly once. * test(app-router): cover metadata RSC redirects The unit CI failure came from a strict dispatch test receiving optional fallback fields with undefined or null values. Those fields are only meaningful when a boundary module is actually selected. Build fallback option objects without absent boundary fields, preserving the existing call shape for ordinary page-level HTTP access fallbacks. Add focused coverage for fromMetadata redirects on RSC requests so metadata document refresh handling cannot accidentally consume the client navigation Flight path. * docs(app-router): update metadata redirect transport docs and drop dead boundary fallback Address review feedback on the metadata streaming error split: - Update the stale APP_PAGE_METADATA_ERROR_MARKER docblock to describe the new three-way transport split (flight digest / HTML refresh meta tag / 307). - Note the intentional minimal-stub HTML divergence from Next.js's full-document insertion in buildMetadataRedirectHtmlResponse. - Drop the redundant resolveAppPageHttpAccessBoundaryComponent fallback in renderAppPageHttpAccessFallback (boundaryModule already covers both paths). * docs(app-router): fix stale metadata redirect docs in head resolver and test reference Address remaining bonk re-review nits: - Update resolveModuleMetadata docblock in app-page-head.ts to describe the three-way transport split (it still claimed redirects ride in the flight payload even for document SSR). - Point the APP_PAGE_METADATA_ERROR_MARKER Next.js test reference at metadata-streaming, which is where the behavioural contracts are ported from. --------- Co-authored-by: James <james@eli.cx> |
||
|
|
330de786c5 |
test(interception): fixtures and E2E specs for 4 dynamic interception suites (#1364 Part B) (#1785)
* test(interception): add fixtures and E2E specs for 4 dynamic interception suites Adds fixture pages and Playwright E2E specs for the 4 Next.js interception route test suites tracked in issue #1364 (Part B): - interception-dynamic-segment: @modal/(.)[username]/[id] and @modal/(.)test-nested/@sidebar — 6 tests (all pass) - interception-dynamic-single-segment: @modal/(.)groups/[id]/new from a multi-segment source route — 5 tests - interception-dynamic-segment-middleware: locale-rewrite middleware + (.)[username]/p/[id] interception — 4 tests - parallel-routes-and-interception-from-root: (...)[locale]/intercepted from the app root — 3 tests Suite 3 middleware rewrite is tightly scoped to /interception-mw/* in the shared app-basic middleware.ts to avoid interfering with other tests. Tests for suites 2/3/4 that depend on the computeInterceptTarget fix (fix/interception-compute-target, Part A) will fail until that branch is merged, which is expected. * test(interception): mark 7 known-failing tests with test.fail() pending Part C The 7 tests across suites 2-4 exercise behavior that Part C (#1364) will fix. Marking them test.fail() keeps CI green while preserving the specs as executable documentation of the expected behavior. |
||
|
|
cf04ca63e3 |
fix(form): DISALLOWED_FORM_PROPS stripping, file input warning, viewport prefetch, pages-router E2E (#1752)
* fix(form): implement DISALLOWED_FORM_PROPS, file input warning, viewport prefetch, and pages-router E2E - Strip method/encType/target props with dev console.error (Gap 1) - Warn in dev when FormData contains a File value (Gap 2) - Add viewport IntersectionObserver prefetch for App Router (Gap 3) - Add prefetch prop (false | null) to FormProps - Add pages-dir fixture and E2E spec for Pages Router form tests (Gap 4) - Fix Rules of Hooks: move useRef/useCallback/useEffect before conditionals - Update unit tests: stub React 19 dispatcher for hook-safe direct rendering Closes #1355 * test(form): fix 'renders a form with method prop' to assert stripping behavior * fix(form): address code-review findings — JSDoc, parity warnings, prefetch dedup - Fix misleading JSDoc: pages-dir prefetch prop "has no effect" (not "emits a warning") - Add dev warning for invalid prefetch value (not null/false) - Add dev warning for replace/scroll/prefetch passed with a function action - Gate viewport-prefetch IntersectionObserver with getPrefetchedUrls() / hasPrefetchCacheEntryForNavigation() dedup check to match link.tsx - Add 5 new test cases covering the three new warning paths * style(form): fix formatting in form.test.ts * fix(form): match Next.js warning text verbatim, reuse useMergedRef, skip prefetch for unsafe actions Addresses bonk review feedback: - Invalid prefetch warning now matches Next.js verbatim: "The `prefetch` prop of <Form> must be `false` or `null`" - Function-action warnings split into the two distinct Next.js messages (one for `prefetch`, one for `replace`/`scroll` with doc links), gated separately to match app-dir/form.tsx - Reuse the shared useMergedRef helper instead of hand-rolling ref merge - Viewport prefetch now early-returns on unsafe actions, matching the submit-path isSafeAction gate - Update form.test.ts assertions to the new verbatim text * fix(form): run dev-mode prop validations before DISALLOWED_FORM_PROPS strip to match Next.js order * fix(form): match Next.js validation ordering and bail for server/client action submitters Addresses bonk review feedback: - Run prefetch/replace/scroll dev validations before the DISALLOWED_FORM_PROPS strip loop, matching upstream app-dir/form.tsx ordering - Bail out of GET interception for React server-action submitters (`name` starting with $ACTION_ID_/$ACTION_REF_, dev-only silent bail) and client-action submitters (`formAction="javascript:..."`), ported from Next.js form.tsx / form-shared.tsx - Add unit tests covering both submitter bail paths * fix(form): match Next.js dev-validation order and gate submitter warnings Addresses bonk re-review parity findings: - Gate the three console.error calls in hasUnsupportedSubmitterAttributes behind the dev check, matching Next.js form-shared.tsx (the bail itself still runs in all environments). Previously they fired in production. - Hoist checkFormActionUrl(action, "action") into the top dev-validation block so it runs first, matching upstream app-dir/form.tsx ordering. * refactor(form): drop dead-code GET method check for upstream parity Addresses bonk re-review finding: the form-level `method` prop is stripped via DISALLOWED_FORM_PROPS and any non-GET submitter `formmethod` override is already caught by hasUnsupportedSubmitterAttributes, so the explicit getEffectiveMethod GET check was dead code. Upstream app-dir/form.tsx has no equivalent check. Remove it (and the now-unused getEffectiveMethod helper) and document why the method is always GET at that point. * fix(form): align viewport prefetch fetch with link.tsx (priority + purpose) Addresses bonk re-review finding: the Form viewport prefetch fetch() omitted the `priority: "low"` and `purpose: "prefetch"` options that link.tsx passes (link.tsx:445-451). Without them, RSC prefetches competed with higher-priority requests and weren't tagged as prefetches in devtools / Sec-Purpose. Align for consistency with the Link prefetch path. * fix(form): thread interception context + mounted slots into viewport prefetch Addresses bonk re-review substantive finding: the viewport prefetch built headers with createRscRequestHeaders() (no interceptionContext, no mounted-slots header) and keyed the prefetched set on the bare rscUrl. The actual navigation (and link.tsx / the router) compute getPrefetchInterceptionContext(href) + getMountedSlotsHeader() and key on encodeCacheKey(rscUrl, interceptionContext). For intercepted target routes or pages with mounted parallel slots, the Form's prefetched cache key wouldn't match the navigation lookup, so the prefetch was a silent cache miss (wasted work, no dedup against a nearby <Link>). Mirror the Link/router computation: build context-aware headers, key the prefetched set on encodeCacheKey, and thread interceptionContext/mountedSlotsHeader through hasPrefetchCacheEntryForNavigation and prefetchRscResponse. * docs(form): note prefetch effect dep-array redundancy is lint-mandated Bonk flagged actionHref as redundant alongside action in the prefetch effect's dep array. It is derived purely from action + the module constant, but the exhaustive-deps lint requires it to be listed since the effect references it. Document this so it isn't mistaken for an oversight. --------- Co-authored-by: James <james@eli.cx> |
||
|
|
06f37e9c1b |
fix: client HMR dev overlay recovery (#1758)
* Fix client HMR dev overlay recovery * Stabilize dev overlay build error recovery test * Harden dev overlay recovery handling |
||
|
|
0ecd05dfe1 |
test(app-router): remove live network from ISR revalidation fixtures (#1756)
* test(app-router): remove live network from ISR revalidation fixtures The App Router ISR revalidation fixtures fetch httpbin.org only to collect next.tags before rendering request ids. That makes CI depend on external network latency even though the tests never assert the remote response body. Use a deterministic data URL for the tagged fetch in the parent and nested fixtures so tag collection still exercises the ISR invalidation path without waiting on an external service. * test: remove incidental external fetches A layout observation unit test and a dynamic-error fixture used external URLs even though their contracts only need fetch observation or no-store policy detection. Those URLs can start unnecessary network work when the test behavior does not depend on a remote response. Use deterministic data URLs so the same fetch branches and cache options are exercised without relying on external network availability. |
||
|
|
fb233b0a34 |
feat: improve dev error overlay source frames (#1746)
* Improve dev overlay stack source mapping * Polish dev error overlay stack display * Add dev overlay code frames * Align dev overlay message spacing * Add dev overlay error copy action * Fix dev overlay knip exports * Polish dev overlay layout * Polish dev error overlay spacing * Fix dev overlay coverage and source frame metadata * Fix dev overlay recovery for RSC HMR errors * Handle Vite HMR build errors in dev overlay * Fix knip * Use Shadow DOM for dev error overlay |
||
|
|
e3090b08fa |
fix: dev overlay browser sourcemap stacks (#1731)
* Add browser runtime sourcemap regression test * Update pnpm lockfile * Fix dev overlay browser sourcemap stacks * Move dev stack sourcemap resolution to server * Fix knip unused stack endpoint export * Fix dev stack sourcemap proxy host handling * test: cover dev stack sourcemap helpers * fix: remove unused sourcemap type export * refactor: share dev stack sourcemap endpoint * chore: trigger ci rerun * fix: cap dev stack sourcemap request body |
||
|
|
f8481ac04d |
fix(css): preserve distinct media filenames for CSS url() assets (#1725)
* fix(css): preserve distinct media filenames for CSS url() assets
Match Next.js `asset/resource` behaviour for CSS `url()` dependencies:
emit them as files under `/_next/static/media/` instead of inlining, and
keep byte-identical sources (e.g. `dark.svg` / `dark2.svg`) as separate
output files instead of letting Vite/Rolldown's content-based asset
dedupe collapse them onto one filename.
There is no config switch to disable that dedupe (vitejs/vite#8632). The
native escape hatch is `this.emitFile({ fileName })`, which is never
deduped. A build-only, client-scoped pair of plugins:
- mark: during the client CSS transform, tag each relative asset
`url()` with a private `?vinext_css_url_asset=<source-basename>`
query — the only durable carrier of per-reference provenance, since
once Rolldown dedupes, the bundle can't tell which reference came
from which source.
- restore: at `generateBundle`, read the marker back, emit a sibling
file under the wanted source name via the `fileName` escape hatch,
rewrite the reference, and strip the marker.
Each reference resolves from its own marker, so split CSS chunks stay
correct without a shared cursor or bundle-iteration-order dependence.
This is a slimmer reimplementation of the approach explored in #1703:
same mark/restore mechanism and public surface, but the restore side
drops the AssetIndexes machinery, filename-reservation loop, and
multi-fallback lookups in favour of a single basename map + emitted-set,
since the sibling-filename construction (source-stem + content-hash +
ext) makes those guarded cases unreachable.
Tests port the Next.js scss/url-global e2e into the committed
`pages-basic` and `app-basic` fixtures (built in isolation), covering
the single-stylesheet two-identical-urls case and cross-chunk provenance.
Co-authored-by: Nathan Nguyen <146415969+NathanDrake2406@users.noreply.github.com>
* fix(css): merge duplicate vite import; drop ReDoS-flagged regex; clarify comments
- tests/css-url-assets.test.ts: merge the two `vite` imports into one
(import/no-duplicates lint error that failed CI Check).
- client-build-config.ts: replace `/\/+$/` trailing-slash strip with a
linear loop so CodeQL stops flagging a (false-positive) ReDoS.
- index.ts: reword the css-url mark transform's `map: null` comment — the
marker shifts columns transiently but is stripped before final output.
- css-url-assets.ts: note the basename-uniqueness assumption in assetsByBase.
Addresses review feedback on #1725.
Co-authored-by: Nathan Nguyen <146415969+NathanDrake2406@users.noreply.github.com>
* test(css): use dedicated minimal fixtures for url() asset tests
The url() asset tests previously added routes to the shared app-basic /
pages-basic fixtures and built those whole apps in isolation. That made
the tests CPU-heavy and, run concurrently under CI, contended with
app-router.test.ts's static-export test enough to push it past its 30s
timeout (it passes comfortably locally and on main; the extra parallel
load was the trigger). The added app-basic route also slightly inflated
every app-basic-wide test (static export renders all routes).
Move the fixtures into dedicated minimal committed apps
(tests/fixtures/css-url-assets-{app,pages}) that build in ~1-2s. Same
observable contract (byte-identical dark.svg/dark2.svg referenced from
one stylesheet, plus the split-chunk provenance case), now isolated from
the heavyweight shared fixtures and fast enough not to contend.
Co-authored-by: Nathan Nguyen <146415969+NathanDrake2406@users.noreply.github.com>
* docs(test): update css-url-assets header to reference the dedicated fixtures
The file header still described the pre-refactor layout (pages-basic /
app-basic paths that no longer exist). Point it at the dedicated
css-url-assets-{pages,app} fixtures. Comment-only.
Co-authored-by: Nathan Nguyen <146415969+NathanDrake2406@users.noreply.github.com>
---------
Co-authored-by: Nathan Nguyen <146415969+NathanDrake2406@users.noreply.github.com>
|
||
|
|
51690038d8 |
fix(pages-router): cancel in-flight nav on gSSP/gSP data redirect (#1465) (#1691)
* fix(pages-router): return __N_REDIRECT JSON for gSSP/gSP data redirects (#1465) When getServerSideProps/getStaticProps returns `{ redirect }` on a client-side `_next/data` navigation, vinext emitted a real HTTP redirect. The client's fetch transparently followed it to the destination's HTML (not a valid data envelope), forcing a hard reload with console errors and leaving navigation on the wrong page. Mirror Next.js: for data requests, reply 200 with `__N_REDIRECT` / `__N_REDIRECT_STATUS` in pageProps. The client router now detects these markers and re-dispatches a fresh client navigation to the destination, which increments the navigation id and supersedes (cancels) the in-flight navigation so the intermediate page never commits. HTML requests still emit a real HTTP redirect. Fixed in both server paths to keep dev/prod parity (see AGENTS.md): - prod: buildPagesRedirectResponse() in pages-page-data.ts branches on isDataReq - dev: writeGsspRedirect() in dev-server.ts branches on isDataReq - client: navigateClientData() handles pageProps.__N_REDIRECT via handleDataRedirect(), preserving push/replace mode - tests: data-endpoint redirect shape (vitest) + client cancellation (e2e) * fix(pages-router): hard-navigate to external __N_REDIRECT destination (#1465) Address review: handleDataRedirect() passed the originating page URL to the hard-navigation fallback for external/non-absolute __N_REDIRECT destinations, which would loop back to the redirecting page (e.g. an OAuth callback at https://example.com/...). Hard-navigate to the destination instead, matching Next.js's handleHardNavigation({ url: destination }). Drop the now-unused fallback parameter and add a server test asserting an external destination is preserved verbatim in the data envelope. |
||
|
|
82b2720701 |
feat(app-router): support useRouter bfcacheId semantics (#1588)
* feat(app-router): implement useRouter bfcacheId semantics * fix(app-router): refine bfcacheId navigation behavior * test(app-router): align bfcache history state expectations * fix(app-router): handle catch-all bfcache identities * fix(app-router): align bfcache context semantics * fix(app-router): avoid hidden bfcache DOM retention * fix(app-router): hydrate bfcache ids from history state * fix(app-router): handle bfcacheId redirects and slots * test(app-router): expect active slot route ids * Fix bfcache id history restoration * Clarify bfcache id sentinel formatting * Address bfcacheId review feedback * Tie bfcacheId sentinels to shared constants * Address bfcacheId review clarity * Tighten bfcache slot identity check * Deduplicate bfcache history sync check * Handle empty bfcache segment ids * Fix intercepted active slot route ids * Clarify bfcacheId form state coverage * Address bfcacheId review clarity * fix(app-router): skip invisible tree-path segments in bfcache identity getVisibleTreePathSegments only stripped route groups, not parallel @slots or "." default segments. Tree paths contain raw filesystem segments, so a slot preceding visible URL segments over-counted the consumed pathname segments and produced a wrong identity prefix. That re-minted the bfcacheId for layouts/slots that actually persisted across parallel-route navigations (e.g. @modal), diverging from Next.js bfcacheId semantics. Delegate to the canonical isInvisibleSegment helper so the filter stays in sync with the route graph, and add a parallel-slot layout regression test (fails without the fix). Also tighten the hydration-map unit test to actually exercise the "no seeding from minted ids" property instead of asserting a local literal, and correct the SSR bfcache context comment. * fix(app-router): move isInvisibleSegment to browser-safe utils The previous commit imported isInvisibleSegment from app-route-graph.ts into app-browser-state.ts, but app-route-graph.ts imports node:path, node:fs and node:crypto. Pulling it into the browser bundle broke client hydration (app-with-src E2E: instrumentation-client and dev-overlay-recovery timed out waiting for hydration / the navigation runtime). Move the pure isInvisibleSegment helper into the browser-safe routing/utils.ts module (already in the client bundle), re-export it from app-route-graph.ts so Node-side import sites keep working, and have app-browser-state.ts import it from utils.js. The app-route-graph type import is now type-only. * refactor(app-router): address bfcacheId review feedback - Reject history entries with a missing bfcache version in the document-scoped version gate (stop coercing null -> 0) so stale ids from older builds / external pushState can't be restored into a fresh document on the version-0 path. - Parse AppElements metadata once per element map per commit and resolve slot bindings via a precomputed Map (was repeated readMetadata parses plus an O(slots^2) find on every navigation commit). - Read single history-state keys without cloning the whole state object; the write path still clones before mutating. - Dedupe the two identical non-negative-safe-integer validators and share path-segment / catch-all helpers via browser-safe routing/utils. - Move tree-path consumed-segment counting into routing/utils and fix optional-catch-all over-counting. - Drop the unused bfcacheIds param from createHistoryStateWithPreviousNextUrl. * refactor(routing): make catch-all segment predicates module-private isCatchAllSegment/isOptionalCatchAllSegment are only used by countConsumedPathnameSegments in the same module; drop the exports to clear knip's unused-export warning. * test(app-router): lock in bfcache version-gate rejection of missing versions Extract the version-gate decision into a pure, exported isHistoryStateBfcacheVersionCurrent(state, currentVersion) in app-history-state (re-exported via app-browser-state); the browser entry's isCurrentBfcacheVersion now delegates to it. Add a direct regression test asserting a history entry with bfcache ids but no version key is NOT treated as current even when the current version is 0 — the exact cross-document stale-id hole that the earlier `?? 0` coercion left open. --------- Co-authored-by: James <james@eli.cx> |
||
|
|
560f270ee6 |
fix: align app router navigation parity (#1705)
* fix: align app router navigation parity * fix(app-router): preserve canonical RSC and hard-nav semantics App Router RSC requests now keep the route pathname shape when adding _rsc. Stripping a trailing slash made trailingSlash routes fetch a non-canonical URL and let the server redirect the client-side RSC request back to the slash form. The MPA navigation scheduler also keys pending hard navigations by both href and history mode so a later replace does not reuse an earlier push closure. Redirect handling keeps the global browser bridge out of passive server rendering while retaining client-side redirect recovery. Adds focused regression coverage for trailingSlash RSC requests, same-href push-to-replace hard navigation, and client redirect guard loops. Updates existing navigation observers to the canonical route URL plus _rsc transport. * fix(app-router): stabilize action and prefetch parity * fix(app-router): reset MPA dedupe after bfcache restore A bfcache-restored App Router document can keep the scheduler's last pending hard-navigation marker after the external navigation already left the page. Reissuing the same href and history mode then entered the suspended MPA path without scheduling another location change. Expose a scheduler reset for the browser lifecycle boundary and call it on persisted pageshow events. Cover the scheduler contract so the same external navigation can run again after reset. * fix(app-router): isolate redirect bridge from passive boundaries Unhandled client redirect recovery currently lives inside every RedirectBoundary, while Pages Router params fallback can keep stale App Router params after an App-to-Pages transition. That makes passive boundaries own browser runtime wiring and lets stale app params win over live Pages Router params. Move the unhandled redirect bridge into BrowserRoot under AppRouterContext, keep RedirectBoundary passive, and prefer live Pages Router params when that compat context exists. Add an App-to-Pages navigation regression fixture. * fix: extract shared decodeRedirectError helper, rename stale test - Add decodeRedirectError() to shims/navigation.ts as the single source of truth for parsing redirect error digests - Replace three private helpers in error-boundary.tsx with shared decodeRedirectError() call - Replace inline digest parsing in AppRouterRedirectBridge with shared decodeRedirectError() call (fixes unsafe decodeURIComponent) - Rename test that claimed to test redirect() but actually tests <Link> navigation * refactor: eliminate unnecessary type assertions - isRedirectError: use in-narrowing instead of as-casts - AppRouterRedirectBridge: remove as after guard already narrows - RedirectErrorBoundary: inline digest access, use in for handled check * ci: retrigger checks * fix: emit noindex for Forbidden/Unauthorized boundaries, extract retryScrollTo helper * docs: clarify decodeRedirectError comment about defensive join |
||
|
|
8ae5abd56c | fix(app-router): preserve forwarded action redirect wrappers (#1582) | ||
|
|
b1f2e57bc4 |
fix(pages): preserve import.meta.url source paths (#1598)
* fix(pages): preserve import.meta.url source paths
Pages Router production bundles previously left direct import.meta.url expressions for the bundler to evaluate from generated output. Server renders could expose dist/server/entry.js, and prerendered Pages output could miss the client entry needed for hydration to replace the server value with the Turbopack-style browser source URL.
The fix rewrites direct import.meta.url reads in user modules to source-file URLs at transform time, keeps new URL asset bases intact, and loads the Pages client entry from the build manifest for Node production and prerender servers.
Regression coverage ports the Next.js import-meta deploy test, including the prerender and hydrated browser path.
* fix(tests): run import.meta browser coverage in e2e
The import-meta regression test launched Playwright from a Vitest integration shard. That shard does not install Playwright browser binaries in CI, so it failed before exercising the runtime behavior.
Move the browser assertion into the existing pages-router-prod Playwright project, keep the lower-boundary server and client chunk assertions in Vitest, and cache the import-meta plugin root paths after an early source-text bailout to address review feedback.
* refactor(import-meta-url): canonicalize each module once per transform
The transform path canonicalized the same module id twice: once in the
eligibility check and again when computing the replacement value, so every
rewritten module paid for two fs.realpathSync.native() syscalls.
The eligibility check now returns the canonical id and threads it through to
the value computation, leaving a single realpath per module.
Also widens the extension match to .cts/.mts/.cjs and documents that the
new URL() skip only covers the bare URL identifier, matching Vite's own
asset-detection scope.
* test(pages): drop unrelated prerender from top-level-await setup
The top-level-await production test ran runPrerender in its setup, but the
assertions pass identically whether the routes are prerendered or rendered on
demand. The step added unrelated build work to a test that is not about
prerendering or client-entry injection.
* fix(pages): share client entry fallback
* fix(import-meta-url): address PR review feedback
- Handle ChainExpression (import.meta?.url) in new URL() asset-detection
skip so the optional-chain variant preserves Vite asset resolution
- Add explicit ChainExpression check in visit() to avoid fragile
reliance on coincident inner-node start/end ranges
- Add test for new URL('./file', import.meta?.url) preservation
- Fix decodeHtmlText entity ordering (& before ") in both
unit and E2E test helpers
- Add doc comments: excluded prefixes rationale, node_modules
early-exit optimization intent
* fix(pages): select client entry by marker in build manifest
findClientEntryFileFromManifest returned the first chunk flagged
isEntry, but a client build can emit several entry chunks (client
entry plus instrumentation/middleware) and manifest order is not
guaranteed. The on-disk fallback already disambiguates by the
vinext-client-entry / vinext-app-browser-entry marker; apply the
same check to the manifest path so both lookups agree, falling back
to the first entry only when nothing is marked.
* refactor(pages): address review feedback on client-entry/import-meta-url
- prod-server: hoist findClientEntryFile out of the manifest if/else
since it already handles a missing manifest via the on-disk fallback
- import-meta-url: document the two known limitations (destructured
access, aliased new URL() base) that match Vite's own behavior
- import-meta-url: drop the dead visit(value.callee) for new URL() —
the callee is always the bare URL identifier
---------
Co-authored-by: James <james@eli.cx>
|
||
|
|
0c1193fd76 |
fix: include i18n context in Pages ISR cache keys (#1698)
Pages Router ISR cache entries were keyed only by pathname. With i18n domain routing, the same pathname can render different content depending on the matched domain and default locale. A cache entry created for one domain could be reused for another domain. Add an i18n cache variant to the Pages ISR key when i18n is configured: - domain:<domain> for matched domain-locales - locale:<locale> for locale-prefix/default-locale routing Adds a Pages i18n production fixture page with getStaticProps + revalidate and verifies example.fr and example.com maintain separate ISR entries for the same pathname. |
||
|
|
f4687b3537 |
fix(app-router): treat module-only Vite ref errors as action-not-found (#1340) (#1668)
When a server action id resolved from request headers carries the `#<exportName>` suffix (e.g. `/app/foo.ts#bar`), `loadServerAction()` splits the id at `#` and calls `requireModule(file)` with the module path only. `@vitejs/plugin-rsc`'s reference-validation virtual module therefore emits `[vite-rsc] invalid server reference '/app/foo.ts'`, omitting the `#<exportName>` portion. The previous match in `isServerActionNotFoundError` compared the full actionId against the Vite error string, so missing/stale references surfaced as a generic 500 instead of the Next.js-compatible 404 with the `x-nextjs-action-not-found` header. Also accept the pre-`#` module-path form for both the dev-time `[vite-rsc] invalid server reference '<id>'` and prod-time `server reference not found '<id>'` error shapes. Adds a focused unit test reproducing the dev-mode case and integration tests asserting that throwing `notFound()` from a server action returns 404 (both fetch and progressive form paths), porting fixture from Next.js: test/e2e/app-dir/actions/app/server/form.js. |
||
|
|
c2f9f5b53f |
fix(app-router): cache pages with revalidate=Infinity/false (#1487) (#1666)
* fix(app-router): cache pages with revalidate=Infinity/false (#1487) App Router pages exporting `revalidate = Infinity` (or `revalidate = false`, which resolves to Infinity in segment config) were re-rendered on every request: the HTML response policy returned `shouldWriteToCache: false`, and the cache-write policy rejected non-finite revalidate values outright. The downstream Cache-Control header was correct (1y s-maxage), but vinext itself never served identical bytes for repeat requests inside the same process. This change treats `revalidate = Infinity` as "cache indefinitely" rather than "never write to cache": - `resolveAppPageHtmlResponsePolicy` now writes the cached HTML/RSC bundle in production and reports `MISS` on first render, matching the standard ISR write flow. Dev mode continues to emit `STATIC` with no cache write. - `resolveAppPageCacheWritePolicy` accepts `Infinity` as a valid revalidate interval; only `null`, `NaN`, and non-positive values reject the write. The MemoryCacheHandler (used by tests and as the default) already handles Infinity correctly: `revalidateAt = now + Infinity` keeps the entry fresh forever until `revalidatePath`/`revalidateTag` invalidates it. Adds two production-server regression tests: - `export const revalidate = 60` returns HIT with identical timestamp on the second request (covers the standard finite case). - `export const revalidate = Infinity` returns HIT with identical timestamp on the second request (covers the previously broken indefinite case). Refs #1487. Other symptoms listed in the issue (`unstable_cache`, fetch cache directives, force-static, force-no-store, on-demand revalidate) are out of scope here and tracked separately. * test(app-page-response): update Infinity revalidate assertions Production + revalidateSeconds: Infinity now writes to the ISR cache (cacheState: "MISS", shouldWriteToCache: true). Update the existing assertion to match, and add a dev-mode case asserting Infinity stays as a no-write STATIC response in development. |
||
|
|
3ff06784a0 |
fix(app-router): forbidden()/unauthorized() escalate past intermediate layouts (#1547) (#1647)
When a deep page called `forbidden()`/`unauthorized()`/`notFound()` and the
intermediate layout segments had no matching boundary file, vinext's
page-error fast path wrapped the resolved root boundary in *all* of the
route's layouts. That left the intermediate layout's chrome ("Dynamic with
Layout") rendered next to the boundary fallback, diverging from Next.js
where the nearest ancestor boundary owns the entire fallback subtree.
`renderPageSpecialError` now resolves the deepest layout-level boundary
via `resolveAppPageParentHttpAccessBoundary` and slices the layouts to
`[0..boundaryLayoutIndex]`, so layouts beneath the boundary owner stop
rendering. When the route-level boundary file lives next to the page
(not aligned to a layout), all layouts are kept as before.
Ported the Next.js regression cases from
`test/e2e/app-dir/forbidden/basic/forbidden-basic.test.ts` and
`test/e2e/app-dir/unauthorized/basic/unauthorized-basic.test.ts`.
|
||
|
|
2d9f3ffc36 |
fix(app-router): accept icons.other as single descriptor (#1492) (#1667)
Next.js's `metadata.icons.other` accepts either a single
`IconDescriptor` or an array of them, but `MetadataHead` iterated
it directly and threw `TypeError: metadata.icons.other is not iterable`
when given a single object. That crash bubbled up into the page
render so no `<link>` tags (favicon, shortcut, apple, custom) reached
the head at all for any page declaring `icons.other` as an object.
Normalize `metadata.icons.other` to an array before iterating, widen
its type to `OtherIconDescriptor | OtherIconDescriptor[]`, and also
forward the optional `type` attribute so descriptors like
`{ rel: "mask-icon", url, type: "image/svg+xml" }` round-trip
correctly. Ported coverage from
`test/e2e/app-dir/metadata-icons/metadata-icons.test.ts`.
Refs cloudflare/vinext#1492
|
||
|
|
76fe5d8b05 |
fix(pages-router): render custom pages/500 on SSR throw (#1676)
When getServerSideProps (or any other render-time code) threw in the production server entry, vinext collapsed the response into a plain "Internal Server Error" text body. Next.js instead renders the user's custom pages/500.tsx (with pages/_error.tsx as a fallback). Match that behaviour so frameworks that rely on custom error pages — and the Next.js getserversideprops deploy suite — see the expected output. The catch block in `_renderPage` now re-renders through the same entry using `pages/500.tsx` (or the `_error` route) with status 500. A new `__isInternalErrorRender` guard prevents infinite recursion if the error page itself throws; data (`/_next/data/...`) requests still short-circuit to the plain text body since they can't return HTML. Mirrors Next.js base-server.ts `renderToResponseImpl` 500 fallback. The dev server already renders custom 500/error pages via `renderErrorPage()` — this aligns prod with that behaviour and adds explicit regression coverage for both paths via a new gssp-throw fixture. Refs #1458 |
||
|
|
5bcbee60e0 |
fix(app-router): isolate global-not-found chunk to fix CSS cascade (#1549) (#1654)
When `app/global-not-found.tsx` is defined, route-miss 404s render that
module standalone (it ships its own `<html>`/`<body>` and replaces the
root layout). Previously the entry registered global-not-found.tsx as a
static `import * as`, which co-located its CSS with the root layout's CSS
in a single chunk. Lightningcss then dropped overlapping declarations
between the two stylesheets as dead code, so any rule global-not-found
defined that the layout also defined was silently removed — breaking the
CSS cascade for 404s.
Switching the entry to `() => import("…/global-not-found.tsx")` puts the
module in its own JS chunk and lets the SSR pipeline (and `Resources`
metadata) treat its CSS independently of the rest of the app. The
fallback renderer now accepts a `loadGlobalNotFoundModule` loader (cached
per worker) instead of an eagerly-imported module.
Also adds a regression test asserting the SSR markup for `/does-not-exist`
links only global-not-found's stylesheet — never the root layout's — so
the cascade can never resolve to the wrong color.
Ported from Next.js: test/e2e/app-dir/initial-css-order/initial-css-order.test.ts
|
||
|
|
15fbe66c47 |
test(pages-router): regression coverage for edge runtime & OG image API routes (#1338) (#1648)
Adds Pages Router fixtures and dev + production integration tests for the
two scenarios reported as failing in the Next.js Deploy Suite:
- `export const config = { runtime: 'edge' }` API route returns a Web
Response (issue #1338 case 1 — edge API routes were observed as 500).
- `next/og` `ImageResponse` with `runtime: 'edge'` inside a Pages Router
API route returns `image/png` (issue #1338 case 2 — OG routes were
observed as 404).
Ports route handlers from `next.js/test/e2e/edge-pages-support/app/pages/api/hello.js`
and `next.js/test/e2e/og-api/app/pages/api/og.js`. The existing
implementation already wires both code paths correctly via
`handlePagesApiRoute` and the `vinext:og-inline-fetch-assets` plugin; this
PR locks in coverage so a Pages Router-only build cannot silently regress
either path.
Refs #1338
|
||
|
|
5e90f16a56 |
fix(app-router): preserve push autoscroll across refresh (#1602)
Calling router.push() and router.refresh() in the same transition could leave vinext on the current page instead of committing the pushed route. That diverged from Next.js and broke the default autoscroll target for the pushed page. The refresh path dispatched a current-route RSC refresh before the nested push transition could finish scheduling, so the refresh superseded the intended navigation. Track App Router push/replace scheduling for the current task, keep refresh cache invalidation, and skip only the overtaking current-route refresh. Add Next parity coverage for refresh, server-action refresh, focus targets, fragments, hoisted children, and metadata routes. |
||
|
|
07be579048 | test(app-router): cover clean replace after search load (#1626) | ||
|
|
53a97e2002 |
fix(navigation): stabilize Pages Router useParams snapshot for SSR (#1639)
`useParams()` / `useSearchParams()` / `usePathname()` from `next/navigation`
work as cross-router compat shims under Pages Router, deriving their state
from `getPagesNavigationContext()` published by `next/router`. React's
`useSyncExternalStore` requires `getServerSnapshot` to return a stable
reference (Object.is) across calls in a single render — otherwise React
logs:
"The result of getServerSnapshot should be cached to avoid an infinite loop"
and may re-render until the snapshots stabilize, which is wasteful and a
hydration-mismatch hazard. The SSR branch of `getPagesNavigationContext`
was returning a fresh object each call.
Cache the shape on a `WeakMap` keyed on the ALS-isolated SSR context, so
the same `{ pathname, searchParams, params }` shape (with reference-stable
`params` / `searchParams`) is returned for every hook call inside a single
request. The WeakMap key is per-request, so concurrency safety is
preserved.
Adds a Vitest unit assertion (reference equality across repeated SSR
calls) and a Playwright regression matching the Next.js deploy-suite case
(`test/e2e/app-dir/use-params/use-params.test.ts` — 'should work on pages
router'): a Pages Router dynamic page using `useParams()` inside a fixture
that also has an `app/` directory must render the matched param after
hydration.
Refs #1466
|
||
|
|
a20c49ead2 |
fix(middleware): stop merging original query into middleware rewrite target (#1632)
* fix(middleware): stop merging original query into middleware rewrite target
Next.js middleware constructs the rewrite-target URL itself — by mutating
`request.nextUrl` or by passing a fresh URL — and whatever search params
that URL carries IS the final query. vinext was silently re-merging the
original request's query on top, which resurrected keys that middleware
had explicitly removed via `searchParams.delete(...)`.
Tests:
- Add `mw-clear-query-params` Pages Router fixture + dev test ported from
Next.js `test/e2e/middleware-rewrites/test/index.test.ts` ("should
clear query parameters").
- Add prod-server tests for the original-query-preservation paths
(middleware now propagates query via `request.nextUrl.clone()` in the
fixture, mirroring Next.js `app/middleware.js`).
- Add App Router parity test for original-query-preservation.
Closes #1342
* test(middleware): address bonk review nits for #1342
- Fix misleading comment in app-router.test.ts that referenced Object.assign
merge semantics — the PR's premise is that vinext should NOT auto-merge.
- Add prod-server regression test for searchParams.delete on rewrite target,
mirroring the dev coverage so both code paths are exercised explicitly.
|
||
|
|
d458b71e6b |
test(link): port Next.js OnNavigate fixture as Pages Router regression (#1357) (#1628)
* test(link): port Next.js OnNavigate fixture as a Pages Router regression Add a local Playwright fixture mirroring `.nextjs-ref/test/e2e/link-on-navigate-prop/shared/OnNavigate.tsx` so the upstream `onNavigate` parity scenarios are exercised against vinext's own Pages Router runtime. The `onNavigate` prop itself was implemented in #1371 and is covered by unit tests in `tests/link-navigation.test.ts`. The nightly Next.js deploy suite (workflow run 26552059147) still surfaces failures under `test/e2e/link-on-navigate-prop/index.test.ts` for the Pages Router half of the suite, where even a plain `<button onClick={() => setIsLocked(...)}>` does not update state. The App Router half of the same suite passes, suggesting a Pages Router-specific hydration regression rather than a missing `onNavigate` implementation. This commit ports the upstream fixture so the same scenarios run against vinext in CI's existing Playwright suite, narrowing the reproduction without depending on the full Next.js deploy harness. Refs #1357. * test(link): address bot review on OnNavigate Pages Router port - Assert `isClicked` / `isNavigated` flip to `true` in the internal-click test by stubbing `history.pushState` so vinext's Link runs its synthetic handlers in full but the URL never commits and the source DOM stays available for state assertions. Split the navigation check into its own test that asserts `#subpage-heading`. - Port the three remaining upstream scenarios from `.nextjs-ref/test/e2e/link-on-navigate-prop/index.test.ts`: modifier-key click, plain external link (alert-driven), and external link with `replace` (history-length check). Add the corresponding `#external-link` and `#external-link-with-replace` anchors to the fixture to match `shared/OnNavigate.tsx`. - Explain in the download-test why the native `click` listener doesn't interfere with React's synthetic onClick / onNavigate. - Switch `setIsLocked((v) => !v)` to `setIsLocked(!isLocked)` to match upstream's exact callback shape (parity port). Refs #1357. |
||
|
|
a1a448a49f |
fix(route-handler): default Path=/ on returned Set-Cookie when merging mutable cookies (#1484) (#1627)
When a route handler called `cookies().set()` alongside a returned
`Response` that carried raw `Set-Cookie` headers without a `Path`
attribute, vinext appended the raw line verbatim. The resulting comma-
joined `set-cookie` view then read like `bar=bar2, baz=baz2` with no
per-cookie `Path=/`, diverging from Next.js whose `appendMutableCookies`
re-runs every returned cookie through `ResponseCookies.set` (which
normalises a missing `path` to `/`).
`applyMutableCookieFallbacks` now ensures each returned cookie carries
an explicit `Path=` attribute, matching Next.js's behaviour in
`packages/next/src/server/web/spec-extension/adapters/request-cookies.ts`.
Ported the Next.js coverage from
`test/e2e/app-dir/actions/app-action.test.ts` ("should support setting
cookies in route handlers with the correct overrides") as
`nextjs-compat/set-cookies.test.ts`.
Fixes #1484
|
||
|
|
bdbd1f833d |
fix(app-router): preserve source identity for intercepted renders (#1589)
* fix(app-router): preserve source identity for intercepted renders Intercepted App Router renders used the visible target pathname as the payload route identity. That let a modal navigation such as /feed to /photos/42 replace the retained source tree with target-scoped route and page ids, so refreshes, server actions, chained modals, and history traversal could lose source page state. The violated invariant is that an intercepted render has separate display and matched-route identities. This commits a fenced render identity helper, keeps source route and page ids stable for retained source-route payloads, leaves the target pathname in interception proof, and includes source context in RSC cache variants. Regression coverage asserts source payload keys, cache variants, modal refresh/action/history flows, and source client/search state preservation. * fix(app-router): normalize intercepted rsc cache source * fix(app-router): harden intercepted cache regeneration * refactor(app-router): share intercepted path normalization * fixup! fix(app-router): preserve source identity for intercepted renders fixup! fix(app-router): preserve source identity for intercepted renders |
||
|
|
8e3438d267 |
fix(app-router): complete default autoscroll page target parity (#1601)
* fix(app-router): complete default autoscroll page target parity App Router page navigations can commit loading shells, final payloads, and superseded payloads out of order. A pending scroll intent that is only keyed by navigation start can be consumed by the wrong committed tree, which makes default page target scrolling and focus timing diverge from Next.js when loading boundaries or rapid navigations are involved. Bind scroll intents to approved render commits, consume only the matching intent from the committed scroll target, and clear hard-navigation failures only for their own intent. Extend the Next.js compat fixture coverage for skipped first children, loading boundary timing, same-page search param changes, focus behavior, horizontal scroll preservation, and stale intent races. * test(app-router): update link navigation scroll intent assertion * fix(app-router): address review — consume scroll intent on no-commit, tighten test - Consume scroll intent before returning no-commit in renderNavigationPayload, preventing the navigateClientSide fallback from scrolling for an abandoned navigation. - Replace waitForTimeout(700) with expect.poll for robust race-condition stability assertion (CI-flake-safe, stronger proof of stability). - Update wrapperless targeting comment to clarify outer function / inner class split. - Add three unit tests covering no-commit cleanup, stale intent rejection, and claim-without-commitId consumption paths. |
||
|
|
b6ac0c9abd |
test(pages-router): regression test for literal form action=action:foo (#1476) (#1613)
* test(pages-router): regression test for literal form action=action:foo (#1476) Adds a Pages Router fixture page with a literal action:foo string in a form action attribute, plus an integration test asserting the attribute survives SSR serialization intact. This guards against future regressions where React 19 or the RSC plugin might strip the literal string because it resembles a server action ID. Ported from Next.js test/e2e/app-dir/action-in-pages-router. * test: clarify action:foo regression test comment Update the comment to accurately describe that this test verifies React 19 SSR preserves literal string action attributes, not server action invocation. This addresses review feedback on the PR adding the regression test for #1476. |
||
|
|
314fbea508 |
fix(app-router): align RSC navigation scroll targeting with Next.js (#1599)
* fix(app-router): align RSC navigation scroll targeting with Next.js App Router RSC navigations scrolled the document after appNavigate resolved, which lost Next.js segment targeting and focus semantics. That left persistent layout offsets and horizontal scroll behaviour coarser than Next.js during normal navigation. Next.js treats scroll as a navigation intent consumed by the newly navigated segment after commit. This adds a small intent state, wraps the page slot in a client scroll target, and keeps the existing hash-only and popstate paths separate. Adds focused Playwright coverage ported from Next.js autoscroll and navigation focus tests. * test(form): stop asserting app router scroll detail * fix(app-router): prevent focus from changing scroll * fix(app-router): share scroll intent across Vite module instances The App Router scroll intent was stored in plain module-level variables in app-router-scroll-state.ts. navigateClientSide stages the intent and the committed AppRouterScrollTarget consumes it, but next/navigation and the scroll-target module can resolve to separate Vite module instances. If the writer and consumer hold different copies, the staged intent is invisible to the consumer and the post-commit scroll/focus silently no-ops. This is the same multi-instance hazard the rest of the navigation state already guards against with Symbol.for globals (see _CLIENT_NAV_STATE_KEY in navigation.ts and AGENTS.md "RSC and SSR Are Separate Vite Environments"). Move the single pending intent and the id counter onto a Symbol.for global so every module instance shares one slot, matching the established navigation state pattern. Function signatures are unchanged, so callers need no edits. Also document why AppRouterScrollTarget must remain a class component: findDOMNode needs a mounted class instance to locate the first rendered DOM node without a wrapper element, mirroring Next's default scroll handler. |
||
|
|
0046e0691a |
fix(app-router): preserve encoded prerender params (#1597)
* fix(app-router): preserve encoded prerender params App Router prerendered pages with encoded dynamic params could be bypassed in production and rendered again with decoded route params. This diverged from Next.js for generateStaticParams values like "sticks & stones". The root cause was split across build and startup. The prerender render path only had the encoded URL, so normal route matching decoded the page params. After preserving encoded params for the render, production startup still seeded the ISR cache under the encoded artifact path while live requests lookup cache entries by the runtime-normalized pathname. Carry trusted prerender params through the prerender-only request path, keep decoded params for dynamicParams validation, and seed prerendered App Router artifacts under the normalized runtime cache key while reading files from their encoded output path. * chore(app-router): clean up prerender header internals Reuse the shared record guard for prerender route param parsing and remove the local duplicate. Keep Next.js INTERNAL_HEADERS as the exact upstream list, with vinext-only internal headers tracked separately but stripped by the same request pipeline. This fixes the CI unit failure while preserving the prerender header trust boundary. Factor repeated Node-header conversion and forwarded-protocol handling in the production server to avoid local assertions and duplicate loops. * fix(app-router): scope prerender params to route pattern Bind the internal prerender route-param header to the route pattern that produced it so encoded render params cannot cross a rewrite boundary into a different dynamic route shape. The App Router handler now only applies encoded prerender params when the payload routePattern matches the final matched route. Otherwise it falls back to the decoded params from normal route matching. Tighten the header parser to validate the payload shape with Object.entries/Object.keys and add a regression for a prerendered source route rewritten to a dynamic target route with a different param name. * fix(app-router): verify prerender params match route Require trusted prerender route params to prove both the final route pattern and the final decoded matched params before using their encoded render values. This prevents same-pattern rewrites from carrying stale prerender params into a different concrete route match, while keeping valid prerender renders encoded and static validation decoded. Add handler regression coverage for same-pattern rewrites and helper coverage for catch-all array comparison. * docs(app-router): clarify prerender route-param trust boundary The prerender route-param readers and serializer carried implicit coupling that read as security verification but actually delegated it. A reader new to readTrustedPrerenderRouteParams could assume it verifies the prerender secret, when verification happens once upstream at prod-server's nodeToWebRequest boundary and every downstream read trusts that. The double read-then-reattach in createAppRscHandler also depends on the secret header surviving filterInternalHeaders, with no note of that coupling. Document the trust contract: secret is verified at the outer boundary, downstream reads operate on already-trusted requests gated by VINEXT_PRERENDER=1, and the secret header must stay out of the internal strip list for the second read to succeed. Also document the empty-params to null serialization contract. No behavior change; comments only. |
||
|
|
a66004c71e |
fix: export static App Router metadata files (#1590)
* fix: export static App Router metadata files * fix: address static metadata export review --------- Co-authored-by: James Anderson <james@eli.cx> |
||
|
|
bca641535b |
fix: resolve relative server action redirects (#1579)
* fix: resolve relative server action redirects Server action redirects could carry dot-relative locations from the action response. Treating those locations as raw browser redirects resolved them from the current document URL and forced a reload, so nested App Router actions could land on the wrong route or lose client state. Resolve server action redirect locations against the action initiation pathname using Next.js directory-style semantics, keep same-origin redirects on the App Router navigation path, and hard-navigate only for external destinations or parse fallback. Adds browser-level coverage ported from the Next.js relative server action redirect test plus focused helper assertions for the URL resolution contract. * chore: rerun ci * chore: rerun ci again |
||
|
|
e5d9c4ca33 |
fix(pages-router): await Promise-shaped gssp props (#1568)
* fix(pages-router): await Promise-shaped gssp props
Next.js explicitly supports a Promise value for `getServerSideProps`
`props`. Pages returning `{ props: Promise.resolve(...) }` previously
rendered empty pageProps because vinext passed the unresolved Promise
straight through to the serializer. Match Next.js' `render.tsx` by
detecting a thenable `props` and awaiting it before continuing.
The fix is applied symmetrically to the dev server gssp dispatch
(`dev-server.ts`) and the prod page-data dispatch (`pages-page-data.ts`)
so the two stay in sync per AGENTS.md. Other return shapes (`notFound`,
`redirect`, `revalidate`, fallback) are untouched.
Closes #1459
* refactor: simplify Promise-shaped props handling
await Promise.resolve(x) already handles both thenables and plain
values, so the .then duck-typing check is unnecessary.
|
||
|
|
37e077dd06 |
fix(pages-router): preserve query string in Link and router.push (#1572)
`parseQueryString` and the parseQuery template embedded in the Pages
Router worker entry both used `url.split("?")[1]`, which silently drops
everything after the second `?`. URLs like `/linker?href=/about?hello=world`
have query `href=/about?hello=world` per RFC 3986, so the previous
implementation truncated `query.href` to `/about` and `<Link>` rendered
without the embedded query string. Switching to an `indexOf`-based slice
keeps the full query value intact.
Closes #1471
|
||
|
|
7ffa2b7f3c |
fix(pages-router): default Cache-Control on gssp responses (#1574)
getServerSideProps responses now ship the Next.js default
`private, no-cache, no-store, max-age=0, must-revalidate` Cache-Control
header in both dev and prod, and forward `res.setHeader('Cache-Control', ...)`
overrides set inside gssp through to the final response.
Mirrors next/src/server/route-modules/pages/pages-handler.ts where a
gssp page resolves to `revalidate: 0` and getCacheControlHeader emits
the same string, gated by `if (!res.getHeader('Cache-Control'))` to
preserve user overrides.
Closes #1461
|
||
|
|
62454ef447 |
fix: private use-cache request data (#1445)
* fix use cache private request data * fix private cache nesting guard |
||
|
|
3d5fc8dd84 |
fix(app-router): hoist inline beforeInteractive scripts above resource hints (#1558)
* fix(app-router): hoist inline beforeInteractive scripts above resource hints (#1557) React Fizz puts every stylesheet, modulepreload, and preload link emitted via React Float above user-rendered <script> children of <head>, so an inline `<Script strategy="beforeInteractive">` placed first in the root layout lands near the bottom of head instead — well after styles begin to parse. This breaks the standard no-flash dark-mode pattern. Capture inline beforeInteractive content via a new BeforeInteractiveContext that the App Router SSR pipeline wraps the rendered tree with. The Script shim registers the escaped innerHTML + nonce + attributes and returns null from render. The stream transform splices the captured tag immediately after `<head ...>` opens — before any chunk that contains React-emitted resource hints. On the client the shim suppresses the inline render when the App Router runtime is present (detected via the new hasAppNavigationRuntimeBootstrap helper) so hydration matches. External-src and non-App-Router rendering paths are unchanged: external scripts still preload + render in source order, and Pages Router falls through to the existing inline render. * fix: address PR feedback - e2e spec now reads `self.__vinextThemeInitRan` (matches the fixture's actual marker; the prior `documentElement.dataset` assertion would always have been undefined) - gate `renderBeforeInteractiveInlineScripts` on a `/^[a-zA-Z][\w.-]*$/` attribute-name regex so unknown developer-supplied prop keys can't break out of the unquoted name position - drop unused `BeforeInteractiveProvider` export — the SSR entry uses `BeforeInteractiveContext.Provider` directly - correct the fixture layout docstring to describe the actual `self.__vinextThemeInitRan` marker and explain why we avoid `<html>` dataset attributes here - consolidate the duplicate CSP-nonce middleware blocks for `/revalidate-test` and `/beforeinteractive-head-ordering` * docs/test: address PR nit feedback - correct the createTickBufferedTransform jsdoc: only `injectHTML` falls back to end-of-stream; `injectAfterHeadOpenHTML` is silently dropped when `<head ...>` is never found in a discoverable chunk (covered by the existing unit test) - drop the dead pre-request scaffolding from the CSP-nonce test; the meaningful assertions live in the second request, so the first one was just `void html` noise |
||
|
|
6920568e62 |
fix(router): route hybrid pages api fallbacks through pages entry (#1446)
Hybrid App Router plus Pages Router production builds entered through the App Router server and only delegated route misses to renderPage. That meant pages/api routes were outside the fallback path and returned the App Router 404 in mixed apps. The generated RSC entry now dispatches /api misses to the Pages server entry handleApiRoute, applies middleware response effects to that API response, and the hybrid SSR bridge re-exports the API dispatcher. Regression coverage ports the Next.js og-api Pages API ImageResponse case at the production boundary. |
||
|
|
81bdd6acd3 |
fix(pages-router): render loading shell for unlisted fallback: true paths (#1414)
* fix(pages-router): render loading shell for unlisted `fallback: true` paths PR #1394 fixed the post-rewrite static-vs-dynamic priority sub-problem from #1331 and explicitly tracked `fallback: true` rewrite behaviour separately. When `getStaticPaths` returns `fallback: true` and a request hits a path not in the pre-rendered list, vinext was SSRing the page fully — running `getStaticProps` and emitting `isFallback: false` in `__NEXT_DATA__`. Next.js instead ships a loading shell with empty props and `useRouter().isFallback === true`, then the client hydrates by fetching the data URL. The upstream regression test `should rewrite to fallback: true page successfully` in `test/e2e/middleware-rewrites/test/index.test.ts` asserts "Loading..." in the SSR HTML, which vinext could not produce. Thread an `isFallback` signal through `resolvePagesPageData`, `buildPagesNextDataScript`, the Pages Router SSR context, and both the prod (`pages-server-entry.ts`) and dev (`dev-server.ts`) renderers: - `getStaticProps` and `getServerSideProps` are skipped on the fallback render (matches Next.js `render.tsx`'s `if (isSSG && !isFallback)` gate around `getStaticProps`). - Data requests (`/_next/data/<buildId>/<page>.json`) still resolve real props so the client can swap in after the shell ships. - `useRouter().isFallback` reads from the SSR context server-side and from `__NEXT_DATA__.isFallback` client-side — both flip to true on the shell render. The dev-server comment that claimed Next.js dev "also renders fully" was incorrect; Next.js's `pages-handler.ts` invokes `doRender` with `isFallback: true` for dev fallback responses, so dev now serves the shell too. Tests: - Updated `tests/pages-router.test.ts` "SSR renders unlisted path with getStaticPaths fallback: true" — previously documented the deviation; now asserts the loading shell, empty props, and `isFallback: true`. - Added a counterpart test that the data URL for the same unlisted path still returns real props from `getStaticProps`. - Added a production-side regression test mirroring the upstream "should rewrite to fallback: true page successfully" expectation. Refs #1331 * test(pages-page-data): include isFallback: false in normalized render assertion The render branch of `resolvePagesPageData` now returns `isFallback` alongside the other fields (added in the parent commit for the `fallback: true` loading shell). Update the strict-shape `toEqual` in the cache-miss unit test so it matches the new return shape. |
||
|
|
ddfdd18a1a |
fix(pages-router): honour shallow prop on next/link (#1413)
Closes the last sub-problem of #1332. The Pages Router `Router.push` / `Router.replace` already short-circuit the `_next/data` fetch when `options.shallow === true`, but `<Link shallow>` never made it to the router because `shallow` was missing from `LinkProps`. The prop silently fell into `...rest`, was spread onto the underlying `<a>`, and the click handler called `Router.push(href, undefined, { scroll, locale })` with no shallow flag — so every `<Link shallow>` did a full server roundtrip and GSSP re-ran. This matches Next.js's deploy-suite fixture `test/e2e/middleware-trailing-slash/app/pages/shallow.js`, which clicks a `<Link href="/sha?hello=world" shallow>` and asserts the GSSP-generated random message stays unchanged afterwards. The fix is mechanical: add `shallow?: boolean` to `LinkProps`, destructure it in the Link component, forward it through `navigatePagesRouterLink` into the router transition options. `shallow` is only included in the router options when explicitly set so existing callers that omit it keep the same options shape. |
||
|
|
576c55d7db |
feat(pages-router): implement _next/data JSON endpoint (#1384)
* feat(pages-router): implement _next/data JSON endpoint
Recognize `/_next/data/<buildId>/<page>.json` in dev and prod request
pipelines. The URL is normalized to the page path BEFORE middleware
runs (matching Next.js' `handleNextDataRequest`), and the server emits
`{ pageProps }` as JSON with `Content-Type: application/json` instead
of the rendered HTML.
Closes #1330
* fix(pages-router): address bigbonk findings on _next/data endpoint
- Delegate JSON envelope construction from the generated entry to the
typed `buildNextDataJsonResponse` helper so the envelope shape lives
in one place (entries stay thin per AGENTS.md).
- Return a JSON 404 from `resolvePagesPageData` when `getStaticPaths`
with `fallback: false` rejects the path for a data request. Without
this, client-side navigations to unlisted slugs got an HTML 404 body
which fails to parse as JSON in the Next.js client router.
- Apply the same JSON-404 gating in the dev server for
`getStaticPaths fallback:false`, `getServerSideProps.notFound`, and
`getStaticProps.notFound` paths.
- Read the dev buildId from the plugin's resolved `nextConfig.buildId`
first so a user-supplied `generateBuildId` is honored, matching the
value embedded into the prod server entry.
- Add coverage for `getStaticProps` data requests and the
`fallback:false` JSON-404 path.
* chore: drop unused NextDataMatch export (knip)
* fix(pages-router): JSON 404 for dev stale buildId; HTML 404 for gSSP/gSP notFound on HTML nav
Follow-ups from bigbonk round 2:
- Dev request pipeline now returns a JSON 404 when `_next/data/<wrong>` is
hit, mirroring the prod-server path. Previously the dev fall-through
served Vite's HTML 404, breaking client routers that try to parse the
body as JSON.
- `resolvePagesPageData` now gates the gSSP/gSP `notFound: true` 404 body
on `isDataReq` so HTML navigations get the HTML 404 page and data
requests get the JSON envelope. Previously both shapes always returned
the JSON body, which surfaced text-content-type mismatches on HTML
navigations through gSSP/gSP `notFound`.
|
||
|
|
fb7a2f795b |
fix(rsc): redirect returns flight payload (200) for RSC navigations (#1390)
* fix(rsc): redirect returns flight payload (200) for RSC navigations When a server component calls `redirect()` and the client makes an RSC navigation request (`.rsc` URL), encode the redirect as a React error chunk inside the flight stream and return HTTP 200 instead of a raw 307. The client's `RedirectErrorBoundary` decodes the `NEXT_REDIRECT;...` digest and performs the navigation — matching Next.js's behavior in `app-render.tsx generateDynamicFlightRenderResult`. `generateMetadata()` redirects now always return 200 with a flight payload (both RSC and SSR document requests), because metadata is suspended/streamed in Next.js — its redirects ride inside the flight body, never the HTTP status line. Page-component redirects on SSR document requests still emit a 307, unchanged. Closes #1347 * fix(rsc): preserve soft-nav and useTransition pending across flight redirect The previous commit returned a 200 + flight payload for RSC `redirect()` responses but left vinext's client-side navigation loop without a way to detect the redirect — the 200 response fell through to React's render, threw at decode time, and hit the catch path's `window.location.href` hard navigation. That tore down the page mid-transition and clobbered `useTransition`'s isPending state. Add an out-of-band header (`X-Vinext-Rsc-Redirect`) that carries the redirect target alongside the flight body. The browser-entry loop now detects the header right after the existing `resolveRscRedirectLifecycleHop` (307-following) check, drains the body, and `continue`s the loop with the new href. The same `pendingRouterState` spans both hops, so the outer transition stays pending continuously — restoring pre-1347 behavior. Also stamp the build-time `X-Vinext-Rsc-Compatibility-Id` header on the flight-redirect response. Without it, `resolveRscCompatibilityNavigationDecision` treats the response as cross-build and hard-navigates before the redirect header is read. Export `MAX_RSC_REDIRECT_DEPTH` from `app-browser-rsc-redirect` so the new header path can reuse the same hop-limit ceiling. |
||
|
|
b06a1a1d04 |
fix(i18n): strip locale prefix for API routes (#1408)
* fix(i18n): strip locale prefix for API routes Mirror Next.js's behaviour where a locale-prefixed request path like `/fr/api/ok` is matched against `pages/api/ok` after stripping the locale segment. Previously the locale prefix was kept through the `/api/` startsWith check, so any locale-prefixed API URL 404'd. Add a small helper `stripI18nLocaleForApiRoute(url, i18nConfig)` in `server/pages-i18n.ts` that mirrors Next.js's `normalizeLocalePath().pathname` (see packages/next/src/shared/lib/i18n/normalize-locale-path.ts) and wire it into all three Pages Router request pipelines so dev/prod parity holds: - dev plugin: `packages/vinext/src/index.ts` - prod (Node): `packages/vinext/src/server/prod-server.ts` - prod (Cloudflare worker entry): `packages/vinext/src/deploy.ts` App Router is intentionally unchanged: Next.js does not support the `i18n` config field with App Router, so App Router route handlers under `app/*/route.ts` never see a locale prefix to strip. Refs #1336 (item 3). Items 2 (sticky locale on client navigations) and 4 (default-locale path normalisation) are being handled in parallel PRs. Tests: - Unit tests for `stripI18nLocaleForApiRoute` covering basic, regional (`nl-NL`), query-preserving, unconfigured-prefix, and null-config cases (`tests/pages-i18n.test.ts`). - Dev integration tests (`tests/features.test.ts`) and prod integration tests (`tests/pages-i18n-prod.test.ts`) that GET `/fr/api/ok` and assert a 200 "ok" response against the existing `pages-i18n-domains` fixture, ported from Next.js's `test/e2e/middleware-redirects/test/index.test.ts` ("should redirect to api route with locale"). - Updates to `tests/deploy.test.ts` assertions for the generated worker-entry template strings. * test(after-deploy): update worker-entry assertion after locale strip The Pages Router worker entry now calls handleApiRoute(request, apiLookupUrl, ctx) instead of using resolvedUrl directly, so this generated-code assertion needs the new variable name. Refs #1336 (item 3). |
||
|
|
2d035d47bd |
fix: await top-level await module imports (#1381)
* fix: make next/document default export a class Next.js's `next/document` default export is `class Document extends React.Component`. vinext's shim exported a function, so user `_document.tsx` files that use the class-based form (the common Next.js idiom: `class MyDocument extends Document`) ended up extending a function. React refuses to call class constructors without `new`, which caused SSR to throw "Class constructor MyDocument cannot be invoked without 'new'" — surfacing in e2e as 500 / empty responses. This is the actual root cause of the "async modules render empty pages" report (issue #1361): the Next.js `test/e2e/async-modules` fixture's `pages/_document.jsx` uses `class extends Document`, and once it crashes the whole page renders empty. Top-level await itself propagates correctly through Rolldown's static-import graph already. Adds a class-based Document shim that mirrors Next.js's signature (static `getInitialProps`, instance `render()`), plus production-build e2e coverage for pages, `_app.tsx`, `getStaticProps`, `getServerSideProps`, API routes, and class-based `_document.tsx`, all using top-level `await`. Closes #1361 Ports from Next.js: test/e2e/async-modules/index.test.ts https://github.com/vercel/next.js/blob/canary/test/e2e/async-modules/index.test.ts * fix(next/document): align Document base class with Next.js's typed contract Addresses Bonk review feedback on PR #1381: - Export DocumentContext / DocumentInitialProps types matching Next.js, so subclasses that delegate via `await Document.getInitialProps(ctx)` receive typed objects. The runtime path is still a stub (the Pages Router renderPage/defaultGetInitialProps chain is not wired up), but the signatures pin the contract. - Declare the default Document class export in next-shims.d.ts so strict-mode consumers don't see `any` for `import Document from "next/document"`. - Add a class-extends regression test in tests/document.test.ts — this is the contract that broke in issue #1361 (the original Next.js `pages/_document.jsx` uses `class extends Document`). - Switch the class generic default from `unknown` to `{}` to match Next.js's `class Document<P = {}>`. - Clarify App Router fixture comment: the upstream Next.js async-modules suite is Pages-Router-only, so this is adapted, not ported verbatim. |
||
|
|
bc586788a2 |
fix(middleware): preserve query params on rewrite (#1375)
* fix(middleware): preserve query params on rewrite When middleware (or a `next.config.js` rewrite) sends a request to a new target, vinext was throwing away the original request's query string, breaking `getServerSideProps`/route-handler `query` for any URL of the form `/rewrite-me?key=value`. Match Next.js's behaviour: `Object.assign(parsedUrl.query, rewrittenParsedUrl.query)` — keep the original query first, let the rewrite-target's query win on key conflicts. Applied to: - middleware rewrites (middleware-runtime.ts) - beforeFiles / afterFiles / fallback config rewrites (prod-server.ts, index.ts, deploy.ts) Closes #1342 References: - https://github.com/vercel/next.js/blob/canary/packages/next/src/server/route-modules/route-module.ts (Object.assign merge) - https://github.com/vercel/next.js/blob/canary/test/e2e/edge-pages-support/index.test.ts (rewrite query expectations) * test(query): document URL-encoded space normalization on rewrite merge Addresses Bonk review observations on #1375: - Add test confirming `%20` → `+` normalization through URLSearchParams is intentional and lossless (downstream consumers re-parse). - Drop unreachable `?? ""` fallback in mergeRewriteQuery; `String#split` always yields a string. |
||
|
|
0fbbe33a2e |
fix: pass params: null (not {}) for non-dynamic routes (#1374)
Next.js exposes `params` as `null` (effectively — via `params || null` or `params ? await params : null` user-side) to route handlers and `getServerSideProps`/`getStaticProps` on routes with no dynamic segments. vinext was passing an empty object, which is truthy and breaks user code that branches on `params`. The fix computes a user-facing params value at each dispatch boundary (`null` when `route.isDynamic === false`, otherwise the matched object) while leaving internal bookkeeping — navigation context, query merging, `useParams()` — on the empty-object shape that those consumers expect. References the Next.js test suite assertions: - test/e2e/app-dir/app-routes/app-custom-routes.test.ts "does not provide params to routes without dynamic parameters" - test/e2e/edge-pages-support/index.test.ts "should have correct query/params on index" Closes #1350 |