fix(pages): preserve module identity with deployment ids - #2012
Conversation
commit: |
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): preserve module identity with deployment ids
Verdict: LGTM. The fix is correct, well-scoped, and consistent with the existing codebase architecture. No blocking issues found.
What the change does
Stops appending ?dpl=<deploymentId> to Pages Router JavaScript URLs (bootstrap, modulepreload, page/_app import URLs) while retaining ?dpl= tagging for managed non-JS assets (CSS). This applies the same native-ESM module-identity rule already established in utils/built-asset-url.ts:24 (PR #2005) to Pages Router asset tags.
Verification I performed
-
Root cause is real and the fix addresses it.
shims/router.ts:1772doesawait import(pageModuleUrl)on the value stored in__NEXT_DATA__.__vinext.pageModuleUrl. When that URL carried?dpl=...but the unqueried bootstrap imported the same chunk relatively, native ESM produced two distinct module identities → a secondhydrateRooton#__next→ the duplicate React root described in the PR body. Removing the query from JS URLs collapses them to one identity. -
Consistent with the established pattern.
built-asset-url.ts:20-24already doeshostType === "js" ? url : appendDeploymentIdQuery(...)for the exact same architectural reason. This PR mirrors it (value.endsWith(".js") ? url : appendDeploymentIdQuery(...)), and the inline comment accurately explains the native-ESM rationale. -
No regression risk for other deployment-id paths.
appendAssetDeploymentIdQuery(fonts/stylesheets inpages-page-response.ts,app-ssr-entry.ts, and the font Link header atpages-page-handler.ts:498) is only used for CSS/font preloads — never forimport()ed JS modules — so the skew-protection query stays where it is safe. Data-fetch skew protection still rides thex-deployment-idheader (shims/internal/pages-data-target.ts:133,shims/router.ts:1501), unaffected by this change. -
href()input is always a raw.js/.cssfilename (tf,entry) with no pre-existing query at the pointvalue.endsWith(".js")runs, so the suffix check is reliable. The__VINEXT_CLIENT_ENTRY__bootstrap also routes throughhref()and correctly stays unqueried, which is precisely what keeps the bootstrap and page modules on one graph. -
Deliberate, documented Next.js divergence. Upstream Next.js does tag JS script/preload URLs with
?dpl=(its webpack runtime rewrites chunk URLs so identities stay aligned). vinext cannot rely on that under native ESM, so this divergence is intentional — matching the precedent in PR #2005. This is the correct call for vinext's architecture. -
Tests pass locally:
vp test run tests/pages-asset-tags.test.ts→ 34 passedvp test run tests/build-optimization.test.ts tests/asset-prefix.test.ts→ 156 passed, including the e2edeployment ID: keeps native ESM URLs on one module identitytest which asserts no?dpl=appears in any built.jsor in served JS asset URLs.vp checkon both changed files → clean (format, lint, types).
Minor (non-blocking) nit
resolveClientModuleUrl's 5th parameter is now _deploymentId (unused), but both call sites in pages-page-handler.ts:463-476 still pass process.env.__VINEXT_DEPLOYMENT_ID || process.env.NEXT_DEPLOYMENT_ID. Keeping the param preserves the signature but the now-dead argument at the call sites is slightly misleading. Optionally drop the param and the two call-site arguments to make it obvious that JS module URLs are intentionally never deployment-tagged. Not required for merge.
|
Review posted to PR #2012. Summary of my findings: Verdict: LGTM — no blocking issuesThe PR removes Why it's correct:
Verification done:
One non-blocking nit raised: |
Summary
_appURLs on one native ESM identity when a deployment ID is configured?dpl=tagging for managed non-JavaScript assets such as CSSmiddleware-generalclient transition where a second queried client-entry module created a second React root and restored the old page after the destination committedRoot cause
Instrumentation showed the destination
/blog/[slug]component rendered and committed successfully on the original root. About 6 ms later, the Pages client entry evaluated again as the queried URLindex-*.js?dpl=..., calledhydrateRoota second time on the same#__nextcontainer, overwrotewindow.__VINEXT_ROOT__, and rendered the original/[id]page.The initial document bootstrap was unqueried while Pages page/module URLs were deployment-queried. Native ESM does not inherit query strings across relative imports, so queried and unqueried URLs formed duplicate module graphs. This applies PR #2005's existing module-identity rule to Pages Router asset tags.
Validation
middleware-general/test/index.test.ts, exact assertion only: with i18n PASS; without i18n PASSmiddleware-general/test/node-runtime.test.ts, exact assertion only: with i18n PASS; without i18n PASSvp test run tests/pages-asset-tags.test.ts tests/build-optimization.test.ts tests/asset-prefix.test.tsvp test run tests/shims.test.ts -t "shares RouterContext across duplicated next/router module instances"vp run vinext#buildvp check