Conversation
loadFile passed an absolute path straight to import() when loading a .js or .ts user script. On Windows that throws ERR_UNSUPPORTED_ESM_URL_SCHEME, because absolute paths there have to be file:// URLs. The branch a few lines above, for .mjs/.cjs/.mts/.cts and --cjs false, already converts the path with pathToFileURL, so only the default branch was affected. The effect on Windows depends on the caller. loadPrePostScripts passes throwError, so --preScript and --postScript fail with a message that points at the user's script rather than at the path conversion. Callers that do not pass throwError get undefined back and the script is silently ignored. Also move the cleanup of the temporary package.json into a finally block. It ran after the await, so any failure to load the script left the generated package.json behind in the user's script directory. That part is not Windows specific: a user script with a syntax error leaves the file behind on every platform. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Loading a
.js(or.ts) user script fails on Windows:loadFilepasses an absolute path straight toimport():On Windows an absolute path has to be a
file://URL. The branch 24 lines above— for
.mjs/.cjs/.mts/.ctsand--cjs false— already does thiscorrectly with
pathToFileURL, so only the default.js/.tsbranch isaffected.
Impact
It depends on whether the caller passes
throwError:loadPrePostScriptsdoes, so--preScript/--postScriptfail with amessage that blames the user's script rather than the path conversion.
undefinedback, and the script is silently skipped.Windows CI didn't catch it because every script under
test/datais.cjs,which takes the branch that already converts correctly.
Second change: clean up the temporary package.json on failure
The
unlinkof the generatedpackage.jsonsat after theawait import(...)inside the same
try, so any load failure skipped it and left the file behindin the user's script directory. I moved it to a
finally.This one is not Windows specific — a user script with a syntax error leaves the
file behind on every platform. Happy to split it into its own PR if you'd
rather keep them separate.
Verification
Added
test/unittests/engineUtilsTest.jswith two tests. They split cleanlyalong the two problems:
.jsuser scriptpackage.jsonmain, both passwith the change. Also confirmed the stray
package.jsonwas left in thescript directory before, and is gone after.
without the fix it fails,
with the fix it passes.
npx eslintandnpx tscare clean.One more small effect: when a user script genuinely has a syntax error, Windows
now reports that
SyntaxErrorinstead of masking it behindERR_UNSUPPORTED_ESM_URL_SCHEME.Not covered: I could not run the full
npm testsuite locally — it drivesreal browsers, and the driver postinstall doesn't complete on my machine. I ran
lint, tsc and the new test file only, plus the Linux runs linked above. Worth a
full CI run before merging.
If it's useful, adding a step to
windows.ymlthat runs a.jsscript (theexisting ones are all
.cjs) would stop this from coming back — I left that outto keep the diff small, but happy to add it.
AI disclosure, per CONTRIBUTING: this change was made with assistance from
Claude. I reviewed every line, reproduced both failures on Windows myself
before and after, and set up the two Linux runs above specifically to check
that the new test really is red without the fix rather than assuming it.