-
-
Notifications
You must be signed in to change notification settings - Fork 307
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Correct esm problem on windows #559
Correct esm problem on windows #559
Conversation
bin/import-or-require.js
Outdated
@@ -8,7 +8,7 @@ module.exports = function importOrRequire(file) { | |||
const ext = extnamePath(file); | |||
|
|||
if (ext === '.mjs' || (ext === '.js' && getPackageType.sync(file) === 'module')) { | |||
return import(file); | |||
return import("file://" + file); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will break if someone uses --require=packageName
. You need to provide the flle://
on relative ESM paths yourself, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still not sure about the --import=packageName
(only find the require and ignore option). Could you please give an example that would break with this fix?
Sorry, you’re right; i meant |
Also, on what node version are you seeing this issue? And what exact tape command are you running? |
This comment has been minimized.
This comment has been minimized.
Here is a minimal repo for reproduction using I am testing with node 12. |
You can’t pass unquoted globs to tools tho - your shell will expand them. Try putting quotes around the glob. If that doesn’t fix it, I’ll take a look at your repro repo shortly. |
I changed to:
With the same results: it works on ubuntu but breaks on windows. |
interesting, ok. it's also notable you're not using |
Indeed, your repo passes fine on my Mac. |
bin/import-or-require.js
Outdated
@@ -8,7 +8,7 @@ module.exports = function importOrRequire(file) { | |||
const ext = extnamePath(file); | |||
|
|||
if (ext === '.mjs' || (ext === '.js' && getPackageType.sync(file) === 'module')) { | |||
return import(file); | |||
return import('file://' + file); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing the same issue on Windows (with node 16.9.1).
I suggest import(pathToFileURL(file).href)
, using const { pathToFileURL } = require('url')
which was added in node 10.12.0. Prior art: unified-engine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS. The error message that I'm getting has a more complete explanation than the message shared in the OP. Running tape test.js
yields:
Error [ERR_UNSUPPORTED_ESM_URL_SCHEME]: Only file and data URLs are supported by the default
ESM loader. On Windows, absolute paths must be valid file:// URLs. Received protocol 'c:'
That test.js
file is an ESM file on C:\
, within a package with { "type": "module" }
, therefore hitting the import()
if-branch as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ljharb See nodejs/node@a084632. The error message was further tweaked in a later commit, but this should suffice to explain it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vweevers that makes sense, but I still need to find a way to repro it on windows (but i don't have a windows box) or a test that would have failed on windows (even though we're not running tests on windows yet)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you take a PR for a basic windows workflow on GitHub Actions (just on latest node to start), with a test? Can split those into 2 PRs if you prefer that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but I prefer not to use the setup-node
action, and I haven't yet adapted https://github.com/ljharb/actions/tree/main/node/install to work on windows yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, in that case I'll only send a PR for a test (if I can get existing tests to pass on Windows).
This is required on Windows if the argument passed to `import()` is an absolute path. Without it `tape test.js` fails if test.js is ESM. Covered by existing tests: `node test/import.js` fails without this. See nodejs/node@a084632
9179395
to
d487add
Compare
@David-Desmaisons thank you so much for bringing the problem to my attention; I'm going to go with the solution in #571, and longer term, we'll look into running CI tests on windows to prevent this kind of problem in the future. |
- [Fix] use `file://` URLs for dynamic `import()` (#571, #559) - [readme] hard wraps bad, soft wraps good - [readme] fix markdown; github still has a rendering bug - [readme] add badges - [meta] Exclude `fs` from browser bundles (#565) - [Deps] update `glob`, `string.prototype.trim` - [Dev Deps] update `eslint`, `@ljharb/eslint-config`, `safe-publish-latest`, `array.prototype.flatmap` - [actions] update codecov uploader - [Tests] handle carriage returns in stack traces on Windows - [Tests] node 17+ smooshes a version number on the end of the stack trace - [Tests] handle v8 6.9 changing an error message
When using tape on windows in a esm module (using
"type": "module"
in the project package.json), this exception is raised:In turns out that making explicit the file protocol in the importOrRequire resolves this problem.