fix(pages): preserve import.meta.url source paths#1598
Conversation
|
@codex review |
commit: |
There was a problem hiding this comment.
Pull request overview
Adds a Vite transform + production server wiring to make direct import.meta.url reads in Pages Router user modules reflect the source module URL (matching Next.js production semantics), while keeping Vite-owned asset resolution (new URL("./asset", import.meta.url)) intact.
Changes:
- Introduces
vinext:import-meta-urlplugin to rewrite directimport.meta.urlto source-file URLs (server) orfile:///ROOT/...URLs (client). - Updates the Pages Router Node production server startup to read the client build manifest and publish
__VINEXT_CLIENT_ENTRY__(and lazy chunks) for prerender/hydration parity. - Adds/extends tests to cover server render, client chunk output, and browser hydration expectations.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/pages-router.test.ts | Adds an end-to-end Pages Router production test asserting server and client import.meta.url values (including hydration) and runs prerender in the fixture setup. |
| tests/import-meta-url.test.ts | Adds focused unit tests for the rewrite logic, including /ROOT normalization and new URL(..., import.meta.url) preservation. |
| packages/vinext/src/server/prod-server.ts | Reads .vite/manifest.json to set __VINEXT_CLIENT_ENTRY__ and compute/set lazy chunks for Pages Router Node prod/prerender runs. |
| packages/vinext/src/plugins/import-meta-url.ts | New plugin implementing AST-based rewriting of direct import.meta.url reads with a new URL() base-argument skip. |
| packages/vinext/src/index.ts | Registers the new plugin in the main vinext plugin list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| transform(code, id) { | ||
| const root = options.getRoot(); | ||
| if (!root) return null; | ||
| const cleanId = cleanModuleId(id); | ||
| if (!isTransformableUserModule(cleanId, root)) return null; | ||
|
|
||
| const environment: ImportMetaUrlEnvironment = | ||
| this.environment?.name === "client" ? "client" : "server"; | ||
| const rewritten = rewriteImportMetaUrl(code, cleanId, root, environment); | ||
| if (!rewritten) return null; |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(pages): preserve import.meta.url source paths
Solid PR. The AST-based rewrite approach is well-designed, the new URL() skip is correct, and the test coverage is thorough (unit tests, integration prod-server test, and E2E). The prod-server changes to expose __VINEXT_CLIENT_ENTRY__ from the build manifest close a real parity gap with the Cloudflare worker path.
A few things worth addressing, roughly ordered by importance:
1. Redundant canonicalizePath (double realpathSync) per module
isTransformableUserModule calls canonicalizePath(id) to check eligibility, then importMetaUrlValue calls canonicalizePath(id) again on the same path to compute the replacement value. Each call hits fs.realpathSync.native(). For modules that pass the check, this is two synchronous syscalls for the same input. Copilot flagged a related concern (check-then-parse ordering), but the actual fix is to thread the canonical id through rather than recomputing it.
2. Adding runPrerender to an unrelated existing test
The existing "Pages Router production + top-level await" test now runs runPrerender in its beforeAll. This is a behavioral change to a test that isn't about import.meta.url. If prerender is needed for the top-level-await test to work correctly with the new __VINEXT_CLIENT_ENTRY__ setup, that's fine — but the commit message and PR description don't mention this. Worth a comment in the test explaining why, or splitting it out.
3. isNewUrlExpression only matches bare URL identifier
The new URL(...) skip doesn't handle new globalThis.URL(...) or new window.URL(...). This is a theoretical gap (Vite's own asset handling has the same limitation), but worth a code comment documenting the intentional scope.
Overall this looks correct and well-tested. The inline comments below have specifics.
| rootPaths: RootPaths, | ||
| environment: ImportMetaUrlEnvironment, | ||
| ): string { | ||
| const canonicalId = canonicalizePath(id); |
There was a problem hiding this comment.
canonicalizePath(id) is called here, but the same id was already canonicalized inside isTransformableUserModule (line 123). Each call does fs.realpathSync.native(). Consider threading the canonical id through from isTransformableUserModule (e.g., have it return the canonical id on success, or restructure the transform hook to canonicalize once and pass the result to both functions) to avoid the redundant syscall.
This isn't a correctness issue, but it's an easy win since every transformable module currently pays for two realpathSync calls.
| } | ||
|
|
||
| function isNewUrlExpression(value: NodeLike): boolean { | ||
| return value.type === "NewExpression" && isIdentifierNamed(value.callee, "URL"); |
There was a problem hiding this comment.
This only matches new URL(...) where the callee is a bare Identifier named URL. It won't match new globalThis.URL(...) or new window.URL(...) (those parse as MemberExpression callees). Worth a brief comment noting the intentional scope — especially since Vite's own new URL() asset detection has the same limitation, so this is by design.
| return value.type === "NewExpression" && isIdentifierNamed(value.callee, "URL"); | |
| function isNewUrlExpression(value: NodeLike): boolean { | |
| // Only matches bare `new URL(...)` — not `new globalThis.URL(...)` or | |
| // `new window.URL(...)` — matching Vite's own asset-detection scope. | |
| return value.type === "NewExpression" && isIdentifierNamed(value.callee, "URL"); | |
| } |
| rootPaths: RootPaths, | ||
| environment: ImportMetaUrlEnvironment, | ||
| ): RewriteResult | null { | ||
| if (!code.includes("import.meta.url")) return null; |
There was a problem hiding this comment.
The code.includes("import.meta.url") check is repeated here after already being checked in the transform hook at line 46 and in the public rewriteImportMetaUrl at line 70. For the plugin's internal path (transform → rewriteImportMetaUrlWithRootPaths), this third check is redundant since transform already bailed out early. It's not harmful, but it's worth noting this only serves the rewriteImportMetaUrl export path.
| const { runPrerender } = await import("../packages/vinext/src/build/run-prerender.js"); | ||
| await runPrerender({ | ||
| root: tmpRoot, | ||
| pagesBundlePath: path.join(outDir, "server", "entry.js"), | ||
| concurrency: 1, | ||
| }); |
There was a problem hiding this comment.
This adds runPrerender to the existing "Pages Router production + top-level await" test block. Is this required for the new __VINEXT_CLIENT_ENTRY__ setup to work correctly during these tests, or is it independent? If it's needed (e.g., the prod server now expects prerendered output), consider adding a brief comment explaining why. Otherwise it reads as an unrelated change snuck into this test block.
| }; | ||
| }; | ||
|
|
||
| type ClientBuildManifest = Parameters<typeof computeLazyChunks>[0]; |
There was a problem hiding this comment.
Using Parameters<typeof computeLazyChunks>[0] to derive the manifest type is clever and avoids duplication, but it creates an implicit coupling — if computeLazyChunks's signature changes, these types silently change too. This is probably fine given they're in the same codebase, but worth noting.
Also, consider whether readClientBuildManifest should validate that the parsed result is actually compatible with computeLazyChunks's expectations more strongly. Currently it does thorough field-level validation which is good.
| ); | ||
| globalThis.__VINEXT_LAZY_CHUNKS__ = lazyChunks.length > 0 ? lazyChunks : undefined; | ||
| } else { | ||
| globalThis.__VINEXT_CLIENT_ENTRY__ = undefined; |
There was a problem hiding this comment.
Setting globalThis.__VINEXT_CLIENT_ENTRY__ = undefined and globalThis.__VINEXT_LAZY_CHUNKS__ = undefined in the else branch is good defensive cleanup. But note the original code only set __VINEXT_LAZY_CHUNKS__ when chunks existed — it never explicitly cleared it. The new behavior is stricter (always sets both globals), which is better.
| if (!id || id.startsWith("\0")) return false; | ||
| if (!path.isAbsolute(id)) return false; | ||
| if (id.includes("/node_modules/") || id.includes("\\node_modules\\")) return false; | ||
| if (!/\.(tsx?|jsx?|mjs)$/.test(id)) return false; |
There was a problem hiding this comment.
This regex allows .mjs but not .mts or .cjs/.cts. .mts files with import.meta.url would be silently skipped. Is that intentional? TypeScript projects using .mts extensions are uncommon but not unheard of.
| } | ||
| // When no script tags are present (no hydration), this test just | ||
| // verifies the HTML page itself is served correctly. | ||
| expect(jsMatch).not.toBeNull(); |
There was a problem hiding this comment.
The test now asserts expect(jsMatch).not.toBeNull() where before it was conditional (if (jsMatch) { ... }). This is a stronger assertion that will catch regressions in client-entry injection — nice improvement.
|
Review submitted successfully. Here's a summary of the feedback posted: Overall: The PR is well-structured. The AST-based Key feedback points:
|
|
Addressed review feedback in c4d53f4 and b76cf19:
The remaining notes (the |
|
/bigbonk review |
There was a problem hiding this comment.
Review: fix(pages): preserve import.meta.url source paths
This PR is well-structured and correct. The AST-based rewrite approach is sound, the new URL() skip logic handles edge cases properly, and the test coverage spans unit, integration, and E2E layers. The prod-server manifest changes close a real hydration gap by mirroring what the Cloudflare worker path already does.
The previous review feedback from ask-bonk and Copilot has been addressed cleanly (double realpathSync fixed, isNewUrlExpression scope documented, redundant checks removed, .mts/.cts extensions added).
A few remaining observations below — none blocking.
| return ranges; | ||
| } | ||
|
|
||
| function isNodeLike(value: unknown): value is NodeLike { |
There was a problem hiding this comment.
Nit: isNodeLike returns true for arrays (since typeof [] === "object"). This means visit will iterate array entries twice in the generic fallback — once via the Array.isArray(child) branch of the parent, and once when the array itself enters visit and hits the Object.entries(value) loop (arrays have numeric string keys).
This is harmless for correctness — the re-traversal just does redundant work — but it could be tightened:
| function isNodeLike(value: unknown): value is NodeLike { | |
| function isNodeLike(value: unknown): value is NodeLike { | |
| return !!value && typeof value === "object" && !Array.isArray(value); | |
| } |
This would also let you drop the Array.isArray(child) branch in the generic walker since arrays would be filtered out by isNodeLike at the top of visit.
| return; | ||
| } | ||
|
|
||
| for (const [key, child] of Object.entries(value)) { |
There was a problem hiding this comment.
Minor: iterating Object.entries(value) on an AST node visits some keys that are never AST children (e.g. computed, optional, raw, value when it's a primitive). The skip-list at line 193 handles the most common metadata keys, but the walker still calls isNodeLike on many primitives per node. Not a real problem for the small AST fragments this plugin processes, but worth noting if this ever needs to handle large modules.
| buildManifest: ClientBuildManifest, | ||
| assetBase: string, | ||
| ): string | undefined { | ||
| for (const entry of Object.values(buildManifest)) { |
There was a problem hiding this comment.
Note: Object.values() iterates in insertion order. If a manifest ever has multiple isEntry: true chunks (unlikely for Pages Router, but theoretically possible), this returns the first one encountered. That's probably fine — just flagging it as an implicit assumption.
| ); | ||
|
|
||
| await buildPagesFixtureToOutDir(tmpRoot, outDir); | ||
| const { runPrerender } = await import("../packages/vinext/src/build/run-prerender.js"); |
There was a problem hiding this comment.
The runPrerender call here is well-motivated — it exercises the __VINEXT_CLIENT_ENTRY__ code path in prod-server.ts which reads from .vite/manifest.json and needs prerendered HTML to verify hydration. Good that the previous review feedback about the other test block's runPrerender was removed while this one was kept.
|
Review submitted successfully. Here's a summary: Approved the PR. The AST-based 4 inline comments posted:
|
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.
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.
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.
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.
NathanDrake2406
force-pushed
the
nathan/import-meta-deploy-suite
branch
from
b76cf19 to
aa6a8d6
Compare
|
/bigbonk review |
There was a problem hiding this comment.
Review: fix(pages): preserve import.meta.url source paths
This is well-designed and correct. The core approach — AST-based rewriting of direct import.meta.url reads with a new URL() base-argument skip — is the right solution. The plugin placement, filtering logic, and test coverage are all solid.
What I verified:
- Internal module safety: vinext's own entry generators (which use
import.meta.url~46 times) run outside Vite's module graph entirely. Generated virtual modules have\0prefixes and are excluded. Runtime helpers don't containimport.meta.url. Multiple independent layers of defense. - Plugin ordering:
og-inline-fetch-assets(enforce: "pre") processesfetch(new URL(..., import.meta.url))patterns first via regex. This plugin (enforce: "post") runs after, and its AST-basednew URL()skip correctly preserves any remaining asset patterns. The two plugins compose cleanly. new URL()skip correctness: Theindex === 1check correctly handles edge cases:new URL(import.meta.url)(single arg, index 0) gets rewritten as expected,new URL("./file", import.meta.url)(second arg, index 1) is preserved.- Symlink canonicalization:
canonicalizePathviarealpathSync.nativewith fallback topath.resolveis correct. The unit test with symlinked roots validates this path. - Prod-server manifest changes:
readClientBuildManifesthas thorough field-level validation. ThefindClientEntryFile+__VINEXT_CLIENT_ENTRY__setup correctly mirrors what the Cloudflare worker entry already does at build time.
Two minor inline observations below — neither blocking.
| } | ||
|
|
||
| function isNodeLike(value: unknown): value is NodeLike { | ||
| return !!value && typeof value === "object"; |
There was a problem hiding this comment.
Previous review flagged that arrays pass isNodeLike (since typeof [] === "object"). I agree this is worth tightening — not for correctness (the walker handles arrays fine via the Array.isArray(child) branch), but because arrays entering visit as top-level values hit the generic Object.entries loop and re-traverse their elements with numeric string keys.
This is harmless for correctness and performance is fine for the small AST fragments this plugin processes, but adding && !Array.isArray(value) would make the intent clearer:
| return !!value && typeof value === "object"; | |
| function isNodeLike(value: unknown): value is NodeLike { | |
| return !!value && typeof value === "object" && !Array.isArray(value); | |
| } |
| if (!id || id.startsWith("\0")) return null; | ||
| if (!path.isAbsolute(id)) return null; | ||
| if (id.includes("/node_modules/") || id.includes("\\node_modules\\")) return null; | ||
| if (!/\.[cm]?[jt]sx?$/.test(id)) return null; |
There was a problem hiding this comment.
Minor: this regex covers .js, .jsx, .ts, .tsx, .cjs, .cjsx, .mjs, .mjsx, .cts, .ctsx, .mts, .mtsx. The x suffix for .ctsx and .mtsx is non-standard (no tooling uses those extensions), though matching them is harmless. Just noting this is slightly broader than intended — no action needed.
|
Review submitted. Here's a summary of what I posted: Approved the PR. The AST-based Key things I verified:
Two minor inline comments (non-blocking):
|
| if ( | ||
| firstSegment === ".next" || | ||
| firstSegment === ".vinext-local-package" || | ||
| firstSegment === "dist" |
There was a problem hiding this comment.
what about if someone uses a custom output directory at some point? should we also include .vinext here? should we just be handling everything gitignored here?
| function transformableModuleCanonicalId(id: string, rootPaths: RootPaths): string | null { | ||
| if (!id || id.startsWith("\0")) return null; | ||
| if (!path.isAbsolute(id)) return null; | ||
| if (id.includes("/node_modules/") || id.includes("\\node_modules\\")) return null; |
There was a problem hiding this comment.
we could just do this against the normalise path that we're normalising below couldn't we?
Real issues worth fixing1. In The new Suggest extracting the Cloudflare logic into a shared helper and using it from both call sites. 2. The
3.
4. Aliasing breaks the asset-detection skip. ```js The 5.
Smaller things
OverallThe core approach (post-transform AST rewrite of bare |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
/bigbonk re-review |
1 similar comment
|
/bigbonk re-review |
There was a problem hiding this comment.
Re-review: fix(pages): preserve import.meta.url source paths
The overall approach is sound — AST-based rewriting of direct import.meta.url with a new URL() skip is the right solution, and the refactoring of manifest/client-entry logic into client-build-manifest.ts is a clear improvement. The test coverage is thorough across unit, integration, and E2E layers.
Previous review feedback has been addressed well (double realpathSync, isNodeLike array guard, scope comments, extension coverage, redundant checks).
Two issues worth addressing before merge, plus some smaller observations.
Issues
1. new URL("./file", import.meta?.url) defeats the asset-detection skip (real bug, edge case)
The isNewUrlExpression handler skips the second argument only when isImportMetaUrlNode(args[index]) returns true (line 228). But import.meta?.url parses as a ChainExpression wrapping a MemberExpression, so isImportMetaUrlNode returns false for the outer node. The recursive walker then descends into the ChainExpression, finds the inner MemberExpression, and rewrites it — turning new URL("./file", import.meta?.url) into new URL("./file", "file:///ROOT/..."). This breaks Vite's asset detection for the optional-chain variant.
The standalone import.meta?.url test passes by accident — the inner MemberExpression shares the same start/end range as the outer ChainExpression, so the replacement range is correct. But inside new URL(), the skip logic fails silently.
This is a genuine edge case (nobody writes import.meta?.url inside new URL() in practice), but the test at line 65 creates a false sense of coverage. Consider either:
- Adding
isImportMetaUrlOrChainedNodethat unwrapsChainExpressionbefore checking - Or at minimum, adding a test that asserts
new URL("./file", import.meta?.url)preserves theimport.meta?.urlbase argument
2. james-elicx's comment on excluded prefixes (line 166) is still open
The hardcoded list [".next", ".vinext", ".vinext-local-package", "dist", "out"] plus config.build.outDir covers the common cases, but if someone configures a custom output directory that isn't a child of the project root, or uses a non-standard name, modules in that directory could get rewritten. The outputDirs mechanism already handles the Vite build.outDir case dynamically, which is good. But the static .vinext entry answers james's question about whether it should be in the list — it should (and is).
The broader question of "should we just handle everything gitignored" is an interesting direction but probably over-engineered for this PR. Worth a doc comment explaining the rationale for the static list + dynamic outDir approach, so future maintainers understand why it's not driven by .gitignore.
Smaller observations
3. decodeHtmlText entity ordering — Both copies (E2E test at line 140, unit test at line 5081) decode " before &. This means a literal &quot; in the HTML would be incorrectly decoded to " (the & gets decoded to &, then the resulting " gets decoded to "). For the JSON shape under test this doesn't matter, but the ordering should be & first, then ", to be correct in general. The E2E version uses replaceAll while the unit test version uses regex — minor inconsistency.
4. node_modules check uses pre-canonicalization path — james-elicx's second comment asked whether the node_modules check (line 145) could use the normalized path computed after canonicalization (line 149) instead of normalizing the raw input separately. The current code normalizes the raw id at line 144, checks node_modules at line 145, then canonicalizes at line 148 and normalizes again at line 149. In theory, a symlinked node_modules path could have a canonical path outside node_modules, making the pre-canonicalization check miss it. In practice this doesn't matter (the isPathWithin(normalizedId, rootPaths.normalizedRoot) check at line 150 would catch it), but checking the canonical path would be more correct. The early check on the raw path is a performance optimization to avoid realpathSync on node_modules paths — worth a comment explaining that intent.
5. Plugin ordering is correct — enforce: "post" after vite:asset is right. The og-inline-fetch-assets plugin runs at enforce: "pre" and processes fetch(new URL(..., import.meta.url)) via regex first. This plugin then runs after and its AST-based new URL() skip preserves remaining asset patterns. The two compose cleanly.
6. findClientEntryFile now includes the on-disk fallback in all paths — This addresses the concern from the previous review about the Node prod-server missing the on-disk fallback that the Cloudflare worker path had. The unified findClientEntryFile in client-build-manifest.ts always tries the manifest first, then falls back to scanning the assets directory. Both the Cloudflare build (index.ts) and the Node prod-server (prod-server.ts) now use the same function.
| if (isNewUrlExpression(value)) { | ||
| const args = nodeArray(value.arguments); | ||
| for (let index = 0; index < args.length; index += 1) { | ||
| if (index === 1 && isImportMetaUrlNode(args[index])) continue; |
There was a problem hiding this comment.
This skip only fires when isImportMetaUrlNode(args[index]) returns true, but import.meta?.url parses as ChainExpression > MemberExpression, so the outer node's type is ChainExpression, not MemberExpression. The check fails, the walker descends into the ChainExpression, finds the inner MemberExpression, and rewrites it.
Result: new URL("./file", import.meta?.url) becomes new URL("./file", "file:///ROOT/..."), defeating the asset-detection preservation.
The standalone import.meta?.url test (line 65 of the test file) passes because the inner MemberExpression shares the same start/end range as the outer ChainExpression, so the replacement range is coincidentally correct. But this code path is not exercised inside a new URL() context.
Consider either unwrapping ChainExpression in isImportMetaUrlNode, or adding a test case for new URL("./file", import.meta?.url) to pin the expected behavior.
| normalizedRoot: string, | ||
| options: { outputDirs?: string[] }, | ||
| ): string[] { | ||
| const prefixes = new Set([".next", ".vinext", ".vinext-local-package", "dist", "out"]); |
There was a problem hiding this comment.
Re james-elicx's earlier question: the static list + dynamic outputDirs approach is reasonable for now. .vinext is already included, and custom output directories are captured via config.build.outDir in configResolved. The .gitignore-based approach would add filesystem reads and parsing complexity for marginal benefit.
Worth a brief comment here explaining the rationale — something like:
| const prefixes = new Set([".next", ".vinext", ".vinext-local-package", "dist", "out"]); | |
| // Static list of known output/build directories that should never have | |
| // import.meta.url rewritten. Custom output directories are added dynamically | |
| // from config.build.outDir in configResolved. Using .gitignore was considered | |
| // but adds unnecessary filesystem overhead for this narrow use case. | |
| const prefixes = new Set([".next", ".vinext", ".vinext-local-package", "dist", "out"]); |
| if (!id || id.startsWith("\0")) return null; | ||
| if (!path.isAbsolute(id)) return null; | ||
| const normalizedInputId = normalizePath(id); | ||
| if (normalizedInputId.includes("/node_modules/")) return null; |
There was a problem hiding this comment.
Re james-elicx's earlier comment about using the normalized path below: the pre-canonicalization check here is a valid performance optimization — it avoids realpathSync for node_modules paths, which are the majority of modules in a typical project. But a symlinked path could theoretically have a canonical path outside node_modules (or vice versa). The isPathWithin check at line 150 provides a second safety net, so this isn't a correctness issue in practice. A brief comment noting this is an early-exit optimization would clarify the intent.
| }); | ||
|
|
||
| function decodeHtmlText(text: string): string { | ||
| return text.replaceAll(""", '"').replaceAll("&", "&"); |
There was a problem hiding this comment.
The decode order is wrong for the general case: &quot; in the source HTML would first have " decoded to ", producing &", then & wouldn't match. But if the source contained &quot;, the correct decode order is & first (producing "), then " to ". Harmless for the JSON under test, but worth flipping the order for correctness:
| return text.replaceAll(""", '"').replaceAll("&", "&"); | |
| return text.replaceAll("&", "&").replaceAll(""", '"'); |
| let prodUrl: string; | ||
|
|
||
| function decodeHtmlText(text: string): string { | ||
| return text.replace(/"/g, '"').replace(/&/g, "&"); |
There was a problem hiding this comment.
Same entity decode ordering issue as the E2E copy — & should be decoded last (or first depending on perspective). The correct order is: decode & first to get raw &, then decode " to get ". Currently " is decoded first.
| return text.replace(/"/g, '"').replace(/&/g, "&"); | |
| return text.replace(/&/g, "&").replace(/"/g, '"'); |
|
Review submitted successfully. Here's a summary of the feedback posted: Overall: The approach is correct and well-tested. Previous review feedback has been addressed. The Two issues flagged:
Smaller observations:
|
- 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
Overview
import.meta.urlin Pages Router modules.import.meta.urlreads in user modules to source-file URLs during Vite transforms.file:///.../sourceURLs, while client builds receive Turbopack-stylefile:///ROOT/...URLs.packages/vinext/src/plugins/import-meta-url.ts,packages/vinext/src/server/prod-server.ts,tests/pages-router.test.tsimport.meta.url.Why
import.meta.urlis module identity. For Next.js compatibility, user code should observe the source module URL, not the generated server entry or bundled output URL. This matters in both SSR HTML and hydrated browser output, because the same component can render once on the server and again from the client page chunk.import.meta.urlis a source-module value.vinext:import-meta-urlto replace direct reads before bundling changes the module identity.new URL("./asset", import.meta.url)remains owned by Vite and existing vinext asset plugins.import.meta.urlwhen it is thenew URL()base argument..vite/manifest.jsonand publishes__VINEXT_CLIENT_ENTRY__, matching the Cloudflare worker injection path.What changed
import.meta.urlcould resolve todist/server/entry.js.file:///.../pages/index.tsxURL./ROOTsource URL.file:///ROOT/pages/index.tsx.vinext build --prerender-allMaintainer review path
packages/vinext/src/plugins/import-meta-url.ts: check the AST traversal, source URL selection, symlink canonicalization, andnew URL()skip.packages/vinext/src/index.ts: check plugin placement beforevinext:og-inline-fetch-assetsstill leaves asset bases available for that pre-transform.packages/vinext/src/server/prod-server.ts: check build-manifest parsing and__VINEXT_CLIENT_ENTRY__setup for Node Pages Router production and prerender.tests/pages-router.test.ts: check the ported Next.js behavior test, including prerender and browser hydration.tests/import-meta-url.test.ts: check low-level guard coverage for/ROOT, real source URLs, and asset base preservation.Validation
vp test run tests/import-meta-url.test.tsvp test run tests/pages-router.test.ts -t "Pages Router import.meta.url in production"vp test run tests/og-inline.test.tsvp checkvp env exec --node 24 ./scripts/run-nextjs-deploy-suite.sh /Users/nathan/Projects/vinext/.refs/nextjs-v16.2.6 --retries 0 -c 1 --debug test/e2e/import-meta/import-meta.test.tsRisk / compatibility
node_modules,dist,.next, and.vinext-local-package.new URL("./asset", import.meta.url)is intentionally left intact so Vite and existing vinext asset transforms continue to own asset resolution.References
import.meta.urluser-code shape under test.import.metabindingimport.meta.urlfrom the source module path./ROOTpath helperfile:///ROOT/..../ROOTURLs.Closes #1505