Repository navigation
Dubious statement regarding reserved specifiers #505
Description
Activity
Worth noting is that this is stated in the Terminology section of
importSpecifiers and may not necessarily apply to the CLI, so I wonder if I should close this issue or we really do need a new error here. Debugging this was quite challenging, so please bear with me. :)It shouldn’t work on any platform - perhaps we just have a bug?
Not working would imply throwing an error if I'm not mistaken.
Reacted by Jordan HarbandRelated to nodejs/node#31710, no? Where, after some back and forth, I finally understood
/is interpreted as the pathname in the url (so should be relative to cwd). PR clarifying the docs are here: nodejs/node#32237. I might be misunderstanding, though 😀Reacted by Evan PlaiceRelated to nodejs/node#31710, no?
No, it's not. This issue is about why there are no guards in place to enforce restrictions on specifiers starting with
/or//.This should fail to resolve.
Agreed there is inconsistency here between the docs and implementation, so we should change one or the other.
Reacted by Derek Lewis, Jordan Harband, jonerer and ExE Boss@guybedford agreed. IMO the implementation should change, otherwise we will have inconsistency between platforms. But I'll try to revive my documentation PR as a stopgap measure
@SimenB not quite. Starting with a
/on MacOS is interpreted as "relative to root", which is to say "absolute". On nodev13.11.0andv14.0.0Reacted by Simen Bekkhus and ExE BossRegarding nodejs/help#2642 (comment) I do not think the import specifier requirements should apply to the CLI execution script so I view this as a bug in node.js for Windows. I point to the web where
<script type="module" src="script.js"></script>is valid butimport 'script.js'is forbidden. So in the web the script source is valid because it uses HTML resolution algorithm. Similarly fornode script.mjsCLI path resolution should apply to the main script being executed.I do think the import specifier rules should apply to
--experimental-loader, this is needed to as the experimental loader supports bare import specifiers.We should be wary with our verbiage and comparisons to HTML vs JS (web and/or node). In JS of the web spec the import specifiers are not relative URL strings, they only consist of a non-base relative URL strings on purpose. That is not true for the HTML attribute (in part due to historical reasons). I think equating the two is untrue and likely will lead to problems. That said, treating the CLI as having historical need to support something is fine.
Reacted by Corey Farrell and ExE BossIMO the implementation should change, otherwise we will have inconsistency between platforms.
I also agree that the implementation should change to re-align w/ what's stated in the documentation.
Perhaps a new error should be made to support this reservation.
The
ERR_INVALID_MODULE_SPECIFIERcode seems appropriate here, but I'd appreciate a more suitable message displayed if the implementation were to use this code.I'm mainly interested in keeping
/reserved so that we can eventually use that to mean package root instead of FS root. By doing this, we'd effectively sidestep any potential debate about the sigil character since there would be no need for one.Reacted by ExE Boss(Note that there’s other problems there, namely that it will be unlikely to get consensus given that “/“ universally means system root - but that’s also a reason to keep it reserved)
Reacted by Derek Lewis and ExE BossI've gone ahead and implemented this specifier restriction in a loader that modifies the default resolve hook to throw an
ERR_INVALID_SPECIFIERerror upon failure. This is what I imagine the expected behavior would be if the implementation were to match what's stated in the docs.https://gist.github.com/DerekNonGeneric/9cadd7bea251dd684e78a273445fd193
This loader can be tested in v14.x using …
node --experimental-loader=./reservedResolve_userland.mjs ./foo.mjs
// foo.mjs import * as ns from '/bar.mjs';
Notice I'm intentionally using a restricted specifier (
/bar.mjs), which will trigger the error. By the way, I'm not suggesting the error message in this loader be the exact error message used by Node core if it is decided that the implementation should change, it's simply derived from what's written in the docs.It might be nice to come to a conclusion on this relatively soon before folks start to get accustomed to the current behavior and that ends up not being how the final implementation turns out.
Reacted by ExE BossIf one of the goals is to have browser interop, we should probably remove this restriction.
Refs: https://html.spec.whatwg.org/multipage/webappapis.html#resolve-a-module-specifier
Disclaimer: I don't know what the initial motivation was for making this reservation.
Do we need to do anything here? Perhaps we could just add some docs to https://nodejs.org/dist/latest-v14.x/docs/api/cli.html#cli_1 about the argument being resolved as a file and not through a CJS or ESM loader?
closing due to lack of activity, feel free to make that docs PR separately or reopen if something needs to be done.
This is an outstanding bug. For one, the error that it throws on Debian is wrong.
ERR_MODULE_NOT_FOUND
Looks to be an ongoing source of confusion for a lot of people.
Further discussion surrounding the intended behavior of this can be found at nodejs/node#49449.
Reacted by ExE Boss
https://nodejs.org/api/esm.html (permalink) states the following.
However, at the very least, module specifiers beginning with
/as arguments to thenodeCLI are working just fine for me on Debian, but not on Windows, which is evident in https://github.com/nodejs/node/issues/32952#issuecomment-616803680.Rather than throwing an error stating the above quote, I am experiencing
ERR_REQUIRE_ESM, which isn't consistent with the documentation. Perhaps a new error should be made to support this reservation.