feat(bundler): patch turbopack import.meta output#5905
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a post-pack step that scans emitted Changes
Sequence Diagram(s)sequenceDiagram
participant Bundler
participant FS as FileSystem
participant Manifest
participant Runtime
Bundler->>FS: Read packed .js files (packResult.files)
Bundler->>Bundler: Scan for Turbopack-style import.meta/url patterns
Bundler->>Bundler: Validate/sanitize relative paths (sanitizeBundleOutputRelativePath)
Bundler->>FS: Overwrite .js with runtime url/dirname/filename expressions
Bundler->>FS: Remove sourceMappingURL tokens from modified .js
Bundler->>FS: Resolve and delete stale sourcemap files inside outputDir
Bundler->>Manifest: Update chunks to exclude deleted .js.map files
Bundler->>Bundler: Produce patched outputFiles list
Runtime->>FS: Load patched .js at runtime (uses rewritten expressions)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying egg with
|
| Latest commit: |
8a1d086
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://13d44e24.egg-cci.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-41245e1b.egg-cci.pages.dev |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Deploying egg-v3 with
|
| Latest commit: |
1c956e4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1b8222c6.egg-v3.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-41245e1b.egg-v3.pages.dev |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to patch Turbopack's import.meta output in bundled chunks, ensuring correct runtime resolution of file paths and URLs. It also includes logic to strip and delete stale source map files. Feedback suggests improving the robustness of the patching regex, optimizing performance by reducing redundant file system scans, and minimizing code bloat in the generated import.meta object by pre-calculating values.
| const BUNDLE_MANIFEST_FILENAME = 'bundle-manifest.json'; | ||
| const DEFAULT_WORKER_CHUNK = 'worker.js'; | ||
| const THROWING_IMPORT_META_URL = | ||
| /\(\(\)\s*=>\s*\{\s*throw\s+new\s+Error\(\s*['"]could not convert import\.meta\.url to filepath['"]\s*\)\s*;?\s*\}\)\s*\(\)/g; |
There was a problem hiding this comment.
The regex for matching the throwing import.meta.url is very brittle as it depends on an exact error message string. If Turbopack updates and slightly changes this message (e.g., "file path" instead of "filepath"), the patch will silently fail. Consider making the message matching more flexible to improve robustness.
| /\(\(\)\s*=>\s*\{\s*throw\s+new\s+Error\(\s*['"]could not convert import\.meta\.url to filepath['"]\s*\)\s*;?\s*\}\)\s*\(\)/g; | |
| /\\(\\(\\)\\s*=>\\s*\\{\\s*throw\\s+new\\s+Error\\(\\s*['"].*?import\\.meta\\.url.*?['"]\\s*\\)\\s*;?\\s*\\}\\)\\s*\\(\\)/g; |
| } | ||
| }); | ||
|
|
||
| const outputFiles = await wrapStep('collect output files', () => this.#collectOutputFiles(absOutputDir)); |
There was a problem hiding this comment.
The code performs multiple recursive file system crawls of the output directory (#collectOutputFiles is called here and again at line 155, while PackRunner.run also performs its own crawl). For large bundles, this redundant I/O can impact build performance. Consider passing the file list returned by PackRunner to the patching method and updating it as files are deleted to avoid re-scanning the disk.
| return `${declarationKind} ${metaName} = { | ||
| get url () { | ||
| return ${urlExpr}; | ||
| }, | ||
| get dirname () { | ||
| return ${chunkFilenameExpr}.replace(/[\\\\/][^\\\\/]*$/, ""); | ||
| }, | ||
| get filename () { | ||
| return ${chunkFilenameExpr}; | ||
| } | ||
| };`; |
There was a problem hiding this comment.
The current implementation of #renderImportMetaObject injects the full chunkFilenameExpr and urlExpr IIFEs into every getter. This causes significant code bloat in the output chunks and results in redundant path calculations every time import.meta.url, dirname, or filename is accessed. Wrapping the patched object in an IIFE to pre-calculate these values would be more efficient and reduce the bundle size.
return `${declarationKind} ${metaName} = (() => {
const chunk = ${chunkFilenameExpr};
const url = ${urlExpr};
return {
get url () { return url; },
get dirname () { return chunk.replace(/[\\/][^\\/]*$/, ""); },
get filename () { return chunk; }
};
})();`;
killagu
force-pushed
the
agent/egg-dev/41245e1b
branch
from
44d0315 to
5b0ab00
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tools/egg-bundler/test/integration.test.ts (1)
249-253: ⚡ Quick winUse
pathToFileURL()as the expected-value oracle.Lines 250-252 rebuild the file URL almost the same way as the production patcher, so the test can miss the same encoding bug in both places. Using Node’s canonical
pathToFileURL(filename).hrefkeeps the assertion independent and strengthens thechunk #?.jscase.Suggested change
-import { fileURLToPath } from 'node:url'; +import { fileURLToPath, pathToFileURL } from 'node:url'; ... function expectedFileUrl(filename: string): string { - const u = new URL('file:///'); - u.pathname = filename.replace(/\\/g, '/'); - return u.href; + return pathToFileURL(filename).href; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/egg-bundler/test/integration.test.ts` around lines 249 - 253, The test helper expectedFileUrl reconstructs file URLs manually in expectedFileUrl(filename) which can mirror production bugs; replace its body to return Node's canonical pathToFileURL(filename).href (import pathToFileURL from 'url') and remove the manual replace(/\\/g,'/') logic so assertions use the independent, standard conversion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/egg-bundler/src/lib/Bundler.ts`:
- Around line 196-213: The generated import.meta shim in `#renderImportMetaObject`
currently recomputes filename/dirname/url on each getter call; instead, snapshot
the resolved chunk path and derived dirname/url once at module evaluation and
have the getters return those cached values. Modify `#renderImportMetaObject` so
it declares consts (e.g., cachedFilename, cachedDirname, cachedUrl) initialized
from chunkFilenameExpr and urlExpr immediately and then define get
filename()/get dirname()/get url() to return the cached constants rather than
recomputing from process.argv/process.cwd() on each access.
---
Nitpick comments:
In `@tools/egg-bundler/test/integration.test.ts`:
- Around line 249-253: The test helper expectedFileUrl reconstructs file URLs
manually in expectedFileUrl(filename) which can mirror production bugs; replace
its body to return Node's canonical pathToFileURL(filename).href (import
pathToFileURL from 'url') and remove the manual replace(/\\/g,'/') logic so
assertions use the independent, standard conversion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33b3d0fc-9c34-4bf3-992f-1598a34335fc
📒 Files selected for processing (2)
tools/egg-bundler/src/lib/Bundler.tstools/egg-bundler/test/integration.test.ts
There was a problem hiding this comment.
Pull request overview
This PR updates @eggjs/egg-bundler to post-process Turbopack/@utoo/pack emitted chunks so import.meta.url/dirname/filename resolve to the runtime chunk path, while also stripping/deleting stale sourcemaps and re-enumerating output files for bundle-manifest.json.
Changes:
- Add a Bundler post-step to patch Turbopack
import.metashims in emitted.jschunks and remove stalesourceMappingURLcomments /.js.mapfiles. - Sanitize/validate emitted output relative paths before generating runtime expressions.
- Re-enumerate output files after patching so
bundle-manifest.jsonreflects the final on-disk outputs (excluding deleted sourcemaps).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tools/egg-bundler/src/lib/Bundler.ts | Adds import.meta patching + sourcemap stripping/deletion and re-collects sanitized output file list for bundle-manifest generation. |
| tools/egg-bundler/test/integration.test.ts | Adds an integration test covering nested chunk patching + sourcemap removal and manifest/file list updates. |
| const patchResult = await wrapStep('patch import.meta output', () => this.#patchImportMetaOutput(absOutputDir)); | ||
| debug( | ||
| 'patched %d import.meta output occurrences and removed %d sourcemaps', | ||
| patchResult.patchCount, | ||
| patchResult.deletedMapCount, |
There was a problem hiding this comment.
run() walks the output directory multiple times: #patchImportMetaOutput() calls #collectOutputFiles(), and then run() calls #collectOutputFiles() again for outputFiles. For large bundles this adds extra I/O; consider having the patch step return an updated file list (or the set of deleted .map files) so the final enumeration can reuse earlier results.
| const urlMatches = content.match(THROWING_IMPORT_META_URL); | ||
| let patched = content.replace(THROWING_IMPORT_META_URL, urlExpr); | ||
|
|
||
| let metaMatches = 0; | ||
| patched = patched.replace(TURBOPACK_IMPORT_META_OBJECT, (_match, declarationKind: string, metaName: string) => { | ||
| metaMatches++; | ||
| return this.#renderImportMetaObject(declarationKind, metaName, dirnameExpr, filenameExpr, urlExpr); | ||
| }); | ||
|
|
||
| const patchesForFile = (urlMatches?.length ?? 0) + metaMatches; |
There was a problem hiding this comment.
patchesForFile can double-count when a file contains a Turbopack import.meta object whose url getter includes the throwing placeholder: the placeholder replacement is counted, but then the entire object is replaced anyway. This makes the debug/metrics misleading and does redundant work; consider deriving the count from the object replacements first and only counting standalone placeholder replacements that remain outside replaced objects.
| const urlMatches = content.match(THROWING_IMPORT_META_URL); | |
| let patched = content.replace(THROWING_IMPORT_META_URL, urlExpr); | |
| let metaMatches = 0; | |
| patched = patched.replace(TURBOPACK_IMPORT_META_OBJECT, (_match, declarationKind: string, metaName: string) => { | |
| metaMatches++; | |
| return this.#renderImportMetaObject(declarationKind, metaName, dirnameExpr, filenameExpr, urlExpr); | |
| }); | |
| const patchesForFile = (urlMatches?.length ?? 0) + metaMatches; | |
| let metaMatches = 0; | |
| const metaPatched = content.replace(TURBOPACK_IMPORT_META_OBJECT, (_match, declarationKind: string, metaName: string) => { | |
| metaMatches++; | |
| return this.#renderImportMetaObject(declarationKind, metaName, chunkFilenameExpr, urlExpr); | |
| }); | |
| const urlMatches = metaPatched.match(THROWING_IMPORT_META_URL); | |
| const patched = metaPatched.replace(THROWING_IMPORT_META_URL, urlExpr); | |
| const patchesForFile = metaMatches + (urlMatches?.length ?? 0); |
killagu
force-pushed
the
agent/egg-dev/41245e1b
branch
from
5b0ab00 to
2e4bba2
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5905 +/- ##
=======================================
Coverage 85.04% 85.04%
=======================================
Files 665 665
Lines 19100 19100
Branches 3716 3716
=======================================
Hits 16244 16244
Misses 2463 2463
Partials 393 393 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
killagu
force-pushed
the
agent/egg-dev/41245e1b
branch
from
2e4bba2 to
068e524
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/egg-bundler/src/lib/Bundler.ts (1)
121-143:⚠️ Potential issue | 🔴 CriticalAdd outputDir cleanup in PackRunner.run() and regression test for stale artifacts.
PackRunner.#collectFiles()performs a full recursive scan ofoutputDir. However,PackRunner.run()only callsfs.mkdir(outputDir, { recursive: true })(line 70), which does not remove pre-existing files. IfoutputDircontains orphaned chunks from a previous build, they will be included inpackResult.files, propagating into bothbundleManifest.chunksandBundleResult.files.Add
fs.rm(outputDir, { recursive: true, force: true })before themkdircall, or ensure the caller clearsoutputDirfirst. Either way, add a regression test that verifies stale artifacts do not leak into the output manifest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/egg-bundler/src/lib/Bundler.ts` around lines 121 - 143, PackRunner.run currently only calls fs.mkdir(outputDir, { recursive: true }) so pre-existing files remain and PackRunner.#collectFiles will pick up stale chunks; before creating the directory, call fs.rm(outputDir, { recursive: true, force: true }) (or otherwise ensure the caller clears the directory) to wipe stale artifacts, then recreate with mkdir; update any callers/tests accordingly and add a regression test asserting that orphaned files in outputDir do not appear in packResult.files, bundleManifest.chunks or BundleResult.files after running PackRunner.run.
🧹 Nitpick comments (1)
tools/egg-bundler/test/integration.test.ts (1)
279-285: ⚡ Quick winAdd one no-
__filenamecase for the relativeargv[1]fallback.This block locks down the missing-
argv[1]path, but the new relative-argv[1]branch is still untested because every other call injectsfilename.Suggested regression case
const fallbackFilename = path.join(tmpOutput, 'worker.js'); const fallbackMetaResult = await runPatchedChunk(urlOnlyFilename, { argv: ['node'] }); expect(fallbackMetaResult).toEqual({ url: expectedFileUrl(fallbackFilename), dirname: tmpOutput, filename: fallbackFilename, }); + + const relativeArgvFilename = path.join(tmpOutput, 'chunks/url-only.js'); + const relativeArgvMetaResult = await runPatchedChunk(urlOnlyFilename, { + argv: ['node', './chunks/url-only.js'], + }); + expect(relativeArgvMetaResult).toEqual({ + url: expectedFileUrl(relativeArgvFilename), + dirname: path.dirname(relativeArgvFilename), + filename: relativeArgvFilename, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/egg-bundler/test/integration.test.ts` around lines 279 - 285, Add a new test case exercising the relative-argv[1] fallback: call runPatchedChunk (same as urlOnlyFilename usage) with argv set to include a relative second element (e.g. ['node', './worker.js']) while still omitting __filename so the code hits the relative-argv[1] branch, then assert the returned object matches the same shape as the existing fallback check (url: expectedFileUrl(fallbackFilename), dirname: tmpOutput, filename: fallbackFilename) — use the same fallbackFilename, urlOnlyFilename, expectedFileUrl and tmpOutput variables to locate the test and mirror the existing assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/egg-bundler/src/lib/Bundler.ts`:
- Around line 187-192: The fallback for process.argv[1] in
_buildImportMetaRuntimeExpressions currently only prefixes process.cwd() for
relative entries, leaving dot segments like "./" or "../" intact; update the
fallbackFilenameExpr to fully normalize/resolve relative paths (e.g., use
path.resolve(process.cwd(), entryArg) or construct a file: URL and use its
pathname) before using it as the filename so filenameExpr, dirnameExpr and
urlExpr match the real runtime chunk path; modify the expression inside
_buildImportMetaRuntimeExpressions (specifically the fallbackEntryLiteral /
fallbackFilenameExpr) to perform this resolution and keep the existing
absolute-path test so absolute inputs remain unchanged.
---
Outside diff comments:
In `@tools/egg-bundler/src/lib/Bundler.ts`:
- Around line 121-143: PackRunner.run currently only calls fs.mkdir(outputDir, {
recursive: true }) so pre-existing files remain and PackRunner.#collectFiles
will pick up stale chunks; before creating the directory, call fs.rm(outputDir,
{ recursive: true, force: true }) (or otherwise ensure the caller clears the
directory) to wipe stale artifacts, then recreate with mkdir; update any
callers/tests accordingly and add a regression test asserting that orphaned
files in outputDir do not appear in packResult.files, bundleManifest.chunks or
BundleResult.files after running PackRunner.run.
---
Nitpick comments:
In `@tools/egg-bundler/test/integration.test.ts`:
- Around line 279-285: Add a new test case exercising the relative-argv[1]
fallback: call runPatchedChunk (same as urlOnlyFilename usage) with argv set to
include a relative second element (e.g. ['node', './worker.js']) while still
omitting __filename so the code hits the relative-argv[1] branch, then assert
the returned object matches the same shape as the existing fallback check (url:
expectedFileUrl(fallbackFilename), dirname: tmpOutput, filename:
fallbackFilename) — use the same fallbackFilename, urlOnlyFilename,
expectedFileUrl and tmpOutput variables to locate the test and mirror the
existing assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2179fdbf-a98e-4904-8fcd-4c21e31ef0fd
📒 Files selected for processing (2)
tools/egg-bundler/src/lib/Bundler.tstools/egg-bundler/test/integration.test.ts
killagu
force-pushed
the
agent/egg-dev/41245e1b
branch
from
068e524 to
b94bcc9
Compare
| const segments = normalized.split('/'); | ||
| if ( | ||
| !normalized || | ||
| path.posix.isAbsolute(normalized) || |
There was a problem hiding this comment.
#sanitizeOutputRelativePath() only checks path.posix.isAbsolute(normalized). That will not treat Windows drive-absolute paths like "C:/foo.js" as absolute, so such paths could slip through as “safe” and later be interpreted as absolute on Windows (e.g., path.join(outputDir, rel) ignoring outputDir). Consider also rejecting win32-absolute patterns (e.g., path.win32.isAbsolute(normalized) or a /^[A-Za-z]:// check after normalization).
| path.posix.isAbsolute(normalized) || | |
| path.posix.isAbsolute(normalized) || | |
| path.win32.isAbsolute(normalized) || |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tools/egg-bundler/src/lib/Bundler.ts (1)
15-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize the fallback entry path before deriving
import.meta.*.Line 16 still builds the fallback filename with
process.cwd() + "/" + entryArg. For values like./worker.jsor../chunks/foo.js, that leaves dot segments infilename, anddirnamebecomes.../.or.../../chunksinstead of the real chunk path this shim is trying to emulate. Please fully resolve that branch before derivingfilename/dirname/url.#!/bin/bash node <<'NODE' const path = require('node:path'); const cwd = '/tmp/out'; for (const entryArg of ['./worker.js', '../chunks/foo.js']) { const currentFilename = `${cwd}/${entryArg}`; const resolvedFilename = path.resolve(cwd, entryArg); console.log(JSON.stringify({ entryArg, currentFilename, currentDirname: currentFilename.replace(/[\\/][^\\/]*$/, ''), resolvedFilename, resolvedDirname: path.dirname(resolvedFilename), }, null, 2)); } NODE🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/egg-bundler/src/lib/Bundler.ts` around lines 15 - 18, The fallback filename expression (IMPORT_META_FALLBACK_FILENAME_EXPR) currently concatenates process.cwd() + "/" + entryArg which leaves dot-segments; change the fallback to normalize/resolve the path before exposing it to IMPORT_META_FILENAME_EXPR and IMPORT_META_URL_EXPR — i.e., compute the absolute filename using a resolution similar to path.resolve(process.cwd(), entryArg) (or inline JS that mimics that behavior) so IMPORT_META_FILENAME_EXPR and IMPORT_META_URL_EXPR receive a fully resolved filename and derived dirname/url become correct without leftover "./" or "../" segments.
🧹 Nitpick comments (1)
tools/egg-bundler/test/integration.test.ts (1)
277-283: ⚡ Quick winAdd a relative
argv[1]regression case.This block only locks down the missing-
argv[1]branch. A['node', './worker.js']assertion here would catch the dot-segment fallback bug before it regresses again.➕ Suggested test addition
const fallbackFilename = path.join(tmpOutput, 'worker.js'); const fallbackMetaResult = await runPatchedChunk(urlOnlyFilename, { argv: ['node'] }); expect(fallbackMetaResult).toEqual({ url: expectedFileUrl(fallbackFilename), dirname: tmpOutput, filename: fallbackFilename, }); + + const dotSegmentFallback = await runPatchedChunk(urlOnlyFilename, { + argv: ['node', './worker.js'], + }); + expect(dotSegmentFallback).toEqual({ + url: expectedFileUrl(fallbackFilename), + dirname: tmpOutput, + filename: fallbackFilename, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/egg-bundler/test/integration.test.ts` around lines 277 - 283, Add a regression test for the case where argv[1] is a relative path: after the existing missing-argv[1] assertion for runPatchedChunk(urlOnlyFilename, { argv: ['node'] }), add another call to runPatchedChunk with argv: ['node', './worker.js'] (using the same tmpOutput/fallbackFilename/expectedFileUrl helpers) and assert the returned meta equals the same url/dirname/filename shape so the dot-segment fallback is validated; reference runPatchedChunk, urlOnlyFilename, tmpOutput, fallbackFilename and expectedFileUrl to locate where to insert the new assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tools/egg-bundler/src/lib/Bundler.ts`:
- Around line 15-18: The fallback filename expression
(IMPORT_META_FALLBACK_FILENAME_EXPR) currently concatenates process.cwd() + "/"
+ entryArg which leaves dot-segments; change the fallback to normalize/resolve
the path before exposing it to IMPORT_META_FILENAME_EXPR and
IMPORT_META_URL_EXPR — i.e., compute the absolute filename using a resolution
similar to path.resolve(process.cwd(), entryArg) (or inline JS that mimics that
behavior) so IMPORT_META_FILENAME_EXPR and IMPORT_META_URL_EXPR receive a fully
resolved filename and derived dirname/url become correct without leftover "./"
or "../" segments.
---
Nitpick comments:
In `@tools/egg-bundler/test/integration.test.ts`:
- Around line 277-283: Add a regression test for the case where argv[1] is a
relative path: after the existing missing-argv[1] assertion for
runPatchedChunk(urlOnlyFilename, { argv: ['node'] }), add another call to
runPatchedChunk with argv: ['node', './worker.js'] (using the same
tmpOutput/fallbackFilename/expectedFileUrl helpers) and assert the returned meta
equals the same url/dirname/filename shape so the dot-segment fallback is
validated; reference runPatchedChunk, urlOnlyFilename, tmpOutput,
fallbackFilename and expectedFileUrl to locate where to insert the new
assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d3445a8-54fe-4f74-a9ac-accb49c5f4e2
📒 Files selected for processing (2)
tools/egg-bundler/src/lib/Bundler.tstools/egg-bundler/test/integration.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/egg-bundler/test/integration.test.ts (1)
277-283: ⚡ Quick winAdd a regression case for missing
process.argv[1]fallback.Line 278 validates relative argv fallback, but not the
argv: ['node']branch that should default to"worker.js". Adding that case would lock in the other half of the fallback behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/egg-bundler/test/integration.test.ts` around lines 277 - 283, Add a regression test that covers the `argv: ['node']` branch to ensure `process.argv[1]` falls back to "worker.js": in the same test that calls `runPatchedChunk(urlOnlyFilename, { argv: ['node', './worker.js'] })`, add another call using `runPatchedChunk(urlOnlyFilename, { argv: ['node'] })` and assert its result equals the same object built with `fallbackFilename`, `expectedFileUrl(fallbackFilename)`, and `tmpOutput` (matching the existing expectation shape) so the `argv: ['node']` fallback is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tools/egg-bundler/test/integration.test.ts`:
- Around line 277-283: Add a regression test that covers the `argv: ['node']`
branch to ensure `process.argv[1]` falls back to "worker.js": in the same test
that calls `runPatchedChunk(urlOnlyFilename, { argv: ['node', './worker.js']
})`, add another call using `runPatchedChunk(urlOnlyFilename, { argv: ['node']
})` and assert its result equals the same object built with `fallbackFilename`,
`expectedFileUrl(fallbackFilename)`, and `tmpOutput` (matching the existing
expectation shape) so the `argv: ['node']` fallback is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad9e72c4-6066-4d16-940e-48cd78398eb6
📒 Files selected for processing (2)
tools/egg-bundler/src/lib/Bundler.tstools/egg-bundler/test/integration.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/egg-bundler/src/lib/Bundler.ts`:
- Around line 46-53: The normalization only replaces path.sep, so backslashes in
POSIX inputs (e.g., "..\\chunk.js") can bypass segment checks; update the
normalization for relativeName in Bundler.ts to replace both '/' and '\' (or
normalize all path separators) before computing normalized and segments, then
keep the existing guards (path.posix.isAbsolute, path.win32.isAbsolute,
segments.some(...), normalized.includes('\0')) to validate; locate the variables
relativeName, normalized, and segments and adjust the normalization step so
segments = normalized.split('/') always sees both slash types normalized to '/'.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a48b3cf-f527-430c-baa3-e4f2a1340e29
📒 Files selected for processing (2)
tools/egg-bundler/src/lib/Bundler.tstools/egg-bundler/test/integration.test.ts
| const BUNDLE_MANIFEST_VERSION = 1; | ||
| const BUNDLE_MANIFEST_FILENAME = 'bundle-manifest.json'; | ||
| const IMPORT_META_FALLBACK_FILENAME_EXPR = | ||
| '(() => { const entryArg = typeof process !== "undefined" && process.argv && process.argv[1] ? process.argv[1] : "worker.js"; if (/^(?:[A-Za-z]:[\\\\/]|\\\\\\\\|\\/)/.test(entryArg)) return entryArg; const cwd = typeof process !== "undefined" && process.cwd ? process.cwd() : "."; const raw = cwd + "/" + entryArg; const parts = []; for (const part of raw.replace(/\\\\/g, "/").split("/")) { if (!part || part === ".") continue; if (part === "..") parts.pop(); else parts.push(part); } return (raw.startsWith("/") ? "/" : "") + parts.join("/"); })()'; |
There was a problem hiding this comment.
IMPORT_META_FALLBACK_FILENAME_EXPR normalizes to POSIX separators by splitting on "/" after replacing backslashes, then rejoins with "/". On Windows this makes the computed filename/dirname differ from Node’s __filename/__dirname (which use backslashes), and the integration test expectations (path.join(...)) will also fail on Windows runners. Consider preserving the platform separator (e.g., detect cwd.includes('\\') and join with \\), or avoid manual joining by using a small require('node:path').resolve(process.cwd(), entryArg) expression in the emitted code.
| '(() => { const entryArg = typeof process !== "undefined" && process.argv && process.argv[1] ? process.argv[1] : "worker.js"; if (/^(?:[A-Za-z]:[\\\\/]|\\\\\\\\|\\/)/.test(entryArg)) return entryArg; const cwd = typeof process !== "undefined" && process.cwd ? process.cwd() : "."; const raw = cwd + "/" + entryArg; const parts = []; for (const part of raw.replace(/\\\\/g, "/").split("/")) { if (!part || part === ".") continue; if (part === "..") parts.pop(); else parts.push(part); } return (raw.startsWith("/") ? "/" : "") + parts.join("/"); })()'; | |
| '(() => { const entryArg = typeof process !== "undefined" && process.argv && process.argv[1] ? process.argv[1] : "worker.js"; if (/^(?:[A-Za-z]:[\\\\/]|\\\\\\\\|\\/)/.test(entryArg)) return entryArg; const cwd = typeof process !== "undefined" && process.cwd ? process.cwd() : "."; return require("node:path").resolve(cwd, entryArg); })()'; |
| const mapPaths = new Set<string>([`${filepath}.map`]); | ||
| const sourceMapUrl = this.#extractSourceMappingUrl(originalContent); | ||
| if (sourceMapUrl && !sourceMapUrl.startsWith('data:')) { | ||
| const resolved = path.resolve(path.dirname(filepath), sourceMapUrl); | ||
| if (this.#isInsideDir(outputDir, resolved)) mapPaths.add(resolved); |
There was a problem hiding this comment.
#deleteStaleSourceMaps will delete any file referenced by sourceMappingURL as long as it resolves inside outputDir (not just *.map). A malformed or unexpected sourceMappingURL could cause deletion of non-sourcemap artifacts in the bundle output. It’s safer to only delete the implicit ${filepath}.map and (if present) an extracted sourceMappingURL target when it ends with .map (or otherwise matches an allowlist).
Summary
Scope
This PR is intentionally limited to the import.meta output patch. It does not include the #5889 externals policy changes.
Tests
Summary by CodeRabbit
Improvements
Tests