fix(scripts): use file URL for ESM sourcemap preload - #6044
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe start command converts ESM source-map support paths to file URLs. Tests cover ESM cluster script selection and verify the ChangesESM startup handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: |
0cf8ac6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://04c41274.egg-cci.pages.dev |
| Branch Preview URL: | https://codex-fix-scripts-windows-im.egg-cci.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #6044 +/- ##
==========================================
+ Coverage 83.08% 83.10% +0.02%
==========================================
Files 730 730
Lines 22474 22474
Branches 4520 4520
==========================================
+ Hits 18673 18678 +5
+ Misses 3297 3293 -4
+ Partials 504 503 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes Windows production startup for ESM apps when egg.typescript enables sourcemaps by ensuring the source-map-support/register.js preload passed to Node’s --import is a file:// URL (avoiding drive-letter paths being parsed as an unsupported URL scheme).
Changes:
- Convert the resolved
source-map-support/register.jspath to afile://URL when using--importfor ESM apps. - Keep the CommonJS preload path unchanged via
--require. - Add a regression unit test covering an ESM app (
"type": "module") withegg.typescript: true.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tools/scripts/src/commands/start.ts | Uses pathToFileURL(...).href when pushing the ESM --import preload for source-map-support. |
| tools/scripts/test/start-unit.test.ts | Adds a regression test asserting ESM startup uses --import file: and adjusts the stubbed server bin to return .mjs for ESM. |
Deploying egg-v3 with
|
| Latest commit: |
0cf8ac6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1a1633bc.egg-v3.pages.dev |
| Branch Preview URL: | https://codex-fix-scripts-windows-im.egg-v3.pages.dev |
2674193 to
d231d0b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
d231d0b to
8cbfc88
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
![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/scripts/test/start-unit.test.ts (1)
23-23: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise
Start.getServerBin()for ESM resolution.
TestStart.getServerBin()overridesStart.getServerBin()and returns themjs/cjsextension from the override. The ESM test attools/scripts/test/start-unit.test.ts:92only checks that double. Delegate tosuper.getServerBin()here or add a direct test onStart.getServerBin().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/scripts/test/start-unit.test.ts` at line 23, Update TestStart.getServerBin() so the ESM test exercises Start.getServerBin() rather than bypassing it with a hardcoded extension; delegate to the superclass implementation while preserving the test fixture’s fake script path behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/scripts/test/start-unit.test.ts`:
- Around line 72-99: Extend the ESM preload test around TestStart.run to
explicitly exercise a Windows drive-style esmBaseDir, independent of the host
platform. Verify that the generated --import argument is a file URL with the
Windows path correctly converted, while preserving the existing POSIX-platform
coverage.
---
Nitpick comments:
In `@tools/scripts/test/start-unit.test.ts`:
- Line 23: Update TestStart.getServerBin() so the ESM test exercises
Start.getServerBin() rather than bypassing it with a hardcoded extension;
delegate to the superclass implementation while preserving the test fixture’s
fake script path behavior.
🪄 Autofix
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 Plus
Run ID: 3a8de3ba-9689-46a3-b5af-b0f2880dea93
📒 Files selected for processing (2)
tools/scripts/src/commands/start.tstools/scripts/test/start-unit.test.ts
| it('preloads source-map-support with a file URL for ESM apps', async () => { | ||
| const esmBaseDir = path.join(homeDir, 'esm-typescript-app'); | ||
| await fs.mkdir(esmBaseDir); | ||
| await fs.writeFile( | ||
| path.join(esmBaseDir, 'package.json'), | ||
| JSON.stringify({ name: 'esm-typescript-app', type: 'module', egg: { typescript: true } }), | ||
| ); | ||
| spawnMock.mockImplementation(() => ({ | ||
| once: vi.fn().mockReturnThis(), | ||
| on: vi.fn().mockReturnThis(), | ||
| unref: vi.fn(), | ||
| disconnect: vi.fn(), | ||
| kill: vi.fn(), | ||
| pid: 1112, | ||
| })); | ||
|
|
||
| await TestStart.run(['--workers=1', esmBaseDir]); | ||
|
|
||
| expect(spawnMock).toHaveBeenCalledTimes(1); | ||
| const [, args] = spawnMock.mock.calls[0] as [string, string[]]; | ||
| expect(args).toContain('/fake/scripts/start-cluster.mjs'); | ||
| const importIndex = args.indexOf('--import'); | ||
| expect(importIndex).toBeGreaterThan(-1); | ||
| // A Windows drive path such as D:\app\register.js is parsed as the | ||
| // unsupported `d:` URL scheme by Node's ESM loader unless it is a file URL. | ||
| expect(args[importIndex + 1]).toMatch(/^file:/); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'windows-(latest|2022|2025)|runs-on:.*windows|matrix.*(os|platform)|start-unit\.test' .Repository: eggjs/egg
Length of output: 1888
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## workflow files"
fd -a '.*\.(yml|yaml)$' .github/workflows 2>/dev/null | sed 's#^\./##' | sort | while read -r f; do
echo "--- $f"
rg -n -C 3 'runs-on|windows-latest|matrix|isCI|isWindowsCI|vitest|start-unit|npm test|scripts' "$f" || true
done
echo "## vitest config"
cat -n vitest.config.ts | sed -n '1,120p'
echo "## test relevant section"
cat -n tools/scripts/test/start-unit.test.ts | sed -n '60,110p'
echo "## package scripts"
python3 - <<'PY'
import json, pathlib
p=pathlib.Path('package.json')
if p.exists():
for k,v in json.loads(p.read_text()).get('scripts',{}).items():
if 'vitest' in k.lower() or 'test' in k.lower().split('-')[-1]:
print(f"{k}: {v}")
PYRepository: eggjs/egg
Length of output: 21057
Add explicit Windows path coverage for ESM preload URLs.
Run CI on windows-latest includes tools/scripts/test/start-unit.test.ts, but esmBaseDir is built with the current platform. A POSIX CI runner still sees POSIX paths. Add a Windows-path cover case or a platform-factory path input so --import URL creation covers Windows drive paths.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 74-77: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(
path.join(esmBaseDir, 'package.json'),
JSON.stringify({ name: 'esm-typescript-app', type: 'module', egg: { typescript: true } }),
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/scripts/test/start-unit.test.ts` around lines 72 - 99, Extend the ESM
preload test around TestStart.run to explicitly exercise a Windows drive-style
esmBaseDir, independent of the host platform. Verify that the generated --import
argument is a file URL with the Windows path correctly converted, while
preserving the existing POSIX-platform coverage.
8cbfc88 to
0cf8ac6
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Motivation
On Windows, egg-scripts passes the resolved source-map-support path directly to Node.js --import for ESM applications. A drive-letter path such as D:\app\node_modules\source-map-support\register.js is parsed as the unsupported d: URL scheme, causing ERR_UNSUPPORTED_ESM_URL_SCHEME during production startup.
Fixes #6041.
Changes
Testing
Summary by CodeRabbit