-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
module: add --module
option
#49295
module: add --module
option
#49295
Conversation
Review requested:
|
This used to exist, as
I think this is possible already? node --eval 'import("./2.mjs?key=value#hash")' This is as short as it is because dynamic So I guess the ask is to make this even shorter and more obvious. But before getting to that, what is the use case for this? Why is this common enough to justify a new CLI flag? If this is an edge case, then the existing solution is arguably good enough. |
If |
What surprising behavior? What hidden errors? Let's please not spread FUD.
What linked issues? I read #49204 but I don’t really see a use case; just “I want to be able to launch Node with a URL” but not why the user wants to do that. I don’t think we should be adding complexity for the sake of adding features, only for adding use cases. What’s the use case for launching Node with a URL? |
AFAICT the linked issue objected
If there are problems that are applicable to this version rather than to
Exactly, this is possible with Supporting But it's import, not direct load. The question is, if we have this for const params = new URL(import.meta.url).searchParams;
if (!params.get('algo'))
throw new Error('algo must be supplied!');
if (import.meta.main) {
console.info(`Oops, this should be imported!\nUsage info: ...`);
process.exit(-14);
}
// [implementation] or become #!/usr/bin/env node
const params = new URL(import.meta.url).searchParams;
// TODO: allow algo to be provided as command line argument
if (!params.get('algo'))
throw new Error('algo must be supplied!');
// [implementation]
if (import.meta.main) {
const autogeneratedKey = makeNewKey();
const hash = myHMAC(process.stdin, autogeneratedKey);
console.log({ autogeneratedKey, hash });
} Assuming So, how do I at least test this part with Node.js, without writing a huge loader hook to somehow set Also, how do I load a any module over |
See the
I would think that your proposal shares these issues? With regard to the last one, yes the query params stuff is a use case but that points toward a different goal, of having Node treat the entry as a URL rather than a path string, which is separate from forcing it to be treated as ESM. There are other ways to solve the “treat as URL” request, such as the options mentioned above, or we can propose new ways. Something to consider is that Node already supports Wasm files, and someone might want the equivalent “interpret the entry as a URL” option for a Wasm entry. So |
Also, if users want to misuse it and bypass type resolution, they will do that anyway. They will find a way like
This is argument against
Agreed. The reasons why it's a combined option are:
IMHO this is still more intuitive and more convenient that resolution algorithm that handles URL specifier differently depending on its protocol. Of course, if I'm missing something and there are more reasons to apply path-oriented resolution here, it's possible to rework it into
Right, this approach is applicable for Wasm entry points, too. The choices I see here are:
This probably wouldn't work. There can be multiple After checking for related issues, I'm linking #46009 as one that would be fixed by this PR, let me know if this is incorrect. |
I don’t see how any of these points address the issue. In a folder with no Yes this is a contrived example but it’s illustrating the point that you have two files in the same folder with the same file extension being treated differently; the only way this application could work is with the knowledge that I think there’s an argument to be made that it would be nice to launch Node with an entry point that’s a URL. I just don’t know if that argument is compelling enough to justify creating a new flag, when you can achieve the same via |
These points mostly address the issue being an issue. What is practical benefit of having same type for same extension in same directory? I can see two factors:
Both are not working by default because
Moreover, this "rule" contradicts with the idea of having Tl;dr I think that for URL-based resolution (that is native for
Simple: main entry must be evaluated as ESM and imports must be evaluated as usual. If there is a chance that it may happen in real life, we can probably detect this and print a warning. This usage doesn't seem to be intentional to me, unless for some reason they want to do some weird trick like (() => {
try { 09; } catch { return; }
/* let's use sloppy mode magic! */
})();
If someone designs an application that relies on the fact that
The alternatives to proposed The proposed option doesn't share these issues because it does not mess with any imports at all, it's only applied to main entry point. Also it wouldn't endorse developers to design "packages that work only with this option", as all imports in package will continue working the same, and importing the package as dependency will also work the same.
It's not the same as it's not the main entry point.
This concern might go offtopic but the idea of using querystring to parametrize a module in JavaScript feels even more natural than "Just importing a function" doesn't always work. Let's say, when we import |
You’re essentially arguing that what many people in that earlier thread found confusing isn’t actually confusing. It’s not a “rule” that we decided upon back then; it was a question of “if we allow this override, will users be confused by how it behaves?” and the consensus was that users would be confused; and the benefit of the override didn’t outweigh the cost of the bad UX. I think that that still applies today. It would be nice if Node’s entries were always treated as URLs, the way they are for
Imports have no way of knowing whether they’re the main entry point. We don’t support The bar to adding new flags should be high, as it’s both bad UX the more we add and it adds downstream complexity, like in this flag’s case how this special “treat as module override” is supposed to carry through into the I would suggest that you take a step back and focus on one use case or the other, and open an issue asking for ideas to achieve it, with some that you’ve considered. Building new features is a search for consensus, both that the problem deserves to be solved and the use case supported, and also agreement that your proposed solution is the best one. This PR seems like a jump straight to the end of the process, which can sometimes work if there are lots of people already in agreement on all of the decisions that you made on the way (both that the use case deserves support and that this is the best solution) but based on the prior art around |
Please point it out where many people found confusing about the behaviour proposed in this PR. What was discussed in that earlier thread is
This is correct and this is why I propose a command line option instead of changing how
This is wrong. Node.js's
We don't support it yet. In fact, I think it would make sense to ship this option together with the introduction of
Please point out which parts of proposed implementation are confusing or too complex; perhaps we can solve the specific issues. For
For
So basically, hook gets
There are two issues linked: #46009 and #49204. Wouldn't opening a new one be a duplicate?
This PR is mainly to discuss this particular solution and its implementation, and to see exactly how complex it is, is it feasible, are there any unobvious nuances (right now I see that it needs adjustments in how I'll look into possibility of making
Agreed. Of course there is no rush, I understand that consensus, enough of time and enough amount of explicit approvals is required; as well as possibility of new alternate solutions, or bugs that can't be fixed with this approach. If you feel like the part of discussion that is about issue itself and alternate solutions should be continued in any of the linked issues or a new one, I wouldn't mind to move. Or maybe you would like to suggest something to get more feedback? Either way, thanks for discussion so far, I hope it will serve as good ground for everyone else to agree, counter, or propose a new suggestion. :) |
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’ll just make this explicit: we shouldn’t have a single CLI flag that does both force the Node entry point to be treated as an ES module and force it to be interpreted as an URL string. The first part was already proposed and rejected in --entry-type
and I don’t find the arguments listed here persuasive in light of the previous discussion.
With regard to forcing the entry to be parsed as a URL string, I’m not opposed per se but I’m unconvinced that we need such a feature. I’m not sure that such an ability is worth adding a new flag for. The discussion in #46009 was more one of surprise that Node didn’t support URL strings, not a suggestion that it begin to do so (since apparently it can’t be done without adding a new flag). No one on that thread suggested adding a new flag to support such a feature.
The benefit is not to help humans disambiguate the type of file – for most JS files, any JS dev can very easily guess if it's CJS or ESM at a glance without looking at the file extension or the related The goal is not to have a implication "same extension + same directory => same type", it's to have a deterministic way to tell if such or such file should be parsed as ESM or CJS.
It's possible to write code that can be parsed as either ESM or CJS, and output different things depending how it's parsed. // a.js
import('./b.js');
console.log(`Hello from ${this===undefined ? 'ESM' : 'CJS'}`); // b.js
require('./a.js'); $ node ./a.js
Hello from CJS
$ node --module ./a.js
Hello from ESM
Hello from CJS If we introduce a way for someone to parse twice the same file, given enough time someone is eventually going to fall in that trap by accident, and it's probably going to be very confusing and hard to even understand what's happening.
CJS is simply not supported with network imports and |
As a user, I rather like this option & how it gets Node into such a WinterCG mode, makes it behave more like how modules behave everywhere else on the planet. It seems like a big win. It also resolves #34049 which is one of the longest saddest issues around, which has had more issues merged into it than is to believed. At a start we have #32316, #33223, #37512, #37848, #41522. And address https://github.com/orgs/nodejs/discussions/37857 . It seems like the opinion is turning against this idea. I'd beg to please let us do something. It keeps seemingly like every option has some kind of problem, and that lets us excuse getting no where. Or we stand on precedent. @GeoffreyBooth:
But to quote a user in 34049,
The resolution in nodejs/modules#300 created a disaster. A huge trail of making it impossible to create esm escripts in node.js. Users have been begging for redress for years. The 5 issues I listed above & long story of 34049 itself are a testament to how clearly & obviously help is wanted. If not this PR... one thing that's still super unclear to me is why
The result of 300 created an impossibility to create esm scripts. It sure seems straightforward & obvious to users to expect |
There are multiple use cases being condensed here:
There’s probably not a single solution that works for all of these. There are many problems with The solution for extensionless script files, like that would get a shebang line, that seems most promising is to ship a second binary like |
I agree that having a separate binary would be a good option. I'm affiliated & partial but #37512 seemed promising.
It just makes me very sad that imagined concerns about users misusing what seems to so many here like a straightforward simple ask that would help improve a half decade old solution is the primary objection. Slap an You're right that there are multiple intertangled of concerns, and your breakdown is great. I realize there's a balance in trying to not create more problems as we go, trying to create consistency & clarity, which requires extreme dilligence. And I'm so happy there are so many sharp knowing minds able to break these problems down. My impatience & sadness over trying to create standalone ESM scripts has been growing for a long time however, and I want the impulse to make happen to have a voice, which I don't see in so many PRs. |
Did i read that right? The bar of adding new binary is lower than adding new command line option?
|
no poll, survey, or community involvement, was used to reach such consensus, IIRC, and this issue and PRs keep piling up since about ever. the energy spent to re-hydrate and re-enforce discussions born around the "must use Other runtimes got this solved, the main project around JS in console can't move forward (ever, apparently) about this topic ... I know I am adding nothing to this issue, but the fact it keeps coming up with actual PRs that "just works" and get rejected is something to follow/study/entertain current state of JS around teams/projects. |
No. A binary is the only solution for the extensionless shell script use case, because many platforms don’t allow flags in shebang lines. We’ve never added a new binary before, so it might very well be a nonstarter more broadly. We would need to find some way to create it such that it doesn’t double the size of the Node.js download, for starters. I’m sure people would raise other concerns. The tone on this thread is feeling more argumentative than productive. The solution proposed in this PR has been proposed before and rejected before, so I suggest you move on. Issues can be used for discussing use cases and ideas for solutions. |
you don't need a binary at all if there is a flag ... you need a regular bash / sh script, executable wherever node lands, that starts node with that flag on ... this argument about double binary needed, but no flag wanted, doesn't really look like a smart argument from the very smart people behind this project, sorry. |
@WebReflection can you please phrase your argument in a way that doesn't sound insulting?
That's already possible, a dedicated flag would simplify said bash script, but if you're fine making your own executable nothing's stopping you to make it work already. But I think most folks asking for this would actually want to not have a bash script. |
Then what, exactly, is the proposed alternative? I guess there is consensus on the fact that this PR addresses two, partially related, issues. This fact comes to be the main blocking reason. Your suggestion is to extract the "entry-url" part of it, which is reasonable and feasible on its own. But then what do we do with the "entry-module" part? Do we reintroduce
Which shebang-supporting platforms don't allow flags in 2023?
This is PR thread and this is about specific solution, so questions I'd like to see addressed here are:
I think it would be more productive to see how we can fix the issue and what is the best solution and then implement the best solution; rather than blocking all possible choices just because each of them has its own cons. |
Yes, an
By “the entry-module part” I assume you mean what It would be nice to have a built-in way to run extensionless ESM scripts, and there are several old issues debating various approaches for this. The binary solution came from one of them; to my recollection it’s the idea that seemed the most feasible, though it’s already possible today if you’re willing to add some BASH code. Search for “shebang ESM” in this repo and you’ll see the various old issues, such as #32316. |
The version of I think if we’re going to have a flag called There would be the question of what to do about This would also set the stage for Node potentially flipping to ESM by default in a semver-major change. Then we’d need a flag to opt out, to return to the legacy behavior: We could add this flag and ship an alternate binary that essentially just runs |
@GeoffreyBooth beside the fact I kinda like the "go big or go home" you suggested, you mentioned me so I'd like to clarify:
it's not me eventually being correct, it's explained in the official gnu documentation but I haven't tested it across all possible OS or WSL although I expect these having an if anyone is willing to give it a shot, my example should just run node and show its version, then exit instead of hanging out foever or throw errors. #!/usr/bin/env -S node -v as complete test: TEST_NODE_ESM='/tmp/node-esm'
echo '#!/usr/bin/env -S node -v' > $TEST_NODE_ESM
chmod +x $TEST_NODE_ESM
$TEST_NODE_ESM
rm $TEST_NODE_ESM |
As I already wrote in #34049 (comment), |
Thanks. Yeah I suspected there was still some strong reason why a flag wouldn’t work all on its own. However I think we can have both: the binary is only required for the shebang use case, whereas the flag covers a whole bunch of others. I would suggest starting with the flag as that’s relatively straightforward and not novel like creating a new binary, and once the flag ships we can investigate the binary option. |
@jirutka apologies for not reading that comment out of a different issue but thanks for sharing that ... like I've said I just tested locally and based my assumption out of official gnu documentation (9.3 there) but I wonder at which version |
#34049 is directly related to this PR (and it’s linked in its description).
The latest version, they simply don’t use GNU implementation of
What, why should they? GNU coreutils (the source of GNU |
@jirutka I haven't read all comments linked to this ... my bad.
for the same reason every other OS solved this issue, in a way or another, or never had it to start with ... are you saying those OSs decided explicitly to never be able to have multiple arguments @ shebang line/definition? edit all I could find is a standard from 5 years ago that suggests |
By every other you mean GNU(/Linux) and OpenBSD…?
They probably didn’t decide that, they just probably didn’t feel the need to add another option and diverge from the standard. I’m not saying this option isn’t useful and others shouldn’t implement it, just stating the fact that it’s not as universally supported as some people in this PR think it is. And I definitely didn’t mean it as an argument against adding |
@WebReflection and @jirutka can you please take this discussion of shebang stuff to #49407 or a new issue? This PR is about a |
Sigh, Alpine Linux might be often used in containers, but OpenBSD and NetBSD definitely not.
Exactly, and the |
By not getting approval do you mean absence of explicit approval, verbal disapproval, or explicit block on the PR?
This sounds great and I'd absolutely like to see the end goal of flipping defaults in semver-major release to be reached.
In any previous issues I couldn't find the reason why we don't want extensionless Wasm anymore. Can you elaborate?
Please elaborate this as well.
Can we generalize this to any files with no explicit Sure, there is this question. |
Block. There are many footguns created by overriding just the entry point and not the larger scope. This has been explained numerous times above and in linked issues. I would consider what was earlier proposed as
It’s not that we don’t want it, it’s just that it’s unclear how to achieve it. Right now, extensionless files are parsed as CommonJS. Early on we had them controlled by the Once we find a solution for how Node should know to interpret extensionless files as ESM, we would need to find another solution for permitting extensionless Wasm. One idea that was proposed was that if an extensionless entry point file failed to parse as a string, Node would fall back to trying to detect if it had magic number (the first few bytes defining the type of the file, like how PNG starts with the letters PNG and Wasm starts with \0ASM). There are also out-of-band solutions, like
I’m not sure we need All I mean is that the entry point string would be interpreted in the same way the value of
I don’t know what you mean. Generalize what to any files with no explicit Currently every
Since this mode is opt-in via flag it wouldn’t break anything yet, which gives the ecosystem time to react and update to this potentially becoming the new default. The |
Another proposal along these lines would be one of the previous hopes that we could switch the default operating mode of Node.js at some point in future to an ESM first behaviour. Effectively Node.js today defaults to treating extensionless entry points as CommonJS and to treating packages without a |
Is there consensus that in that "new defaults" mode by default extensionless file is ES module?
Why don't we support extensionless files as ESM right now without flags? How about adding it before flipping the defaults so the adaptation can go smoother for the ecosystem? Also, what about unknown non-empty extensions? Should there be any difference in running
Errm, this is self-contradictory. Why support for an extremely niche use case should become default behaviour. 😄
All of this sound good to me. Two more points to clarify:
My opinion on how to deal with itWe should give developers a choice. All users should have awareness, and end users should have easier way to avoid breakage. jq '. += if (has("type") | not) then { type: "commonjs" } end' I'd raise a question "but how to undo it?" but it's also feasible. Also, we have to keep developers informed that some kind of action is required from them, and to keep users informed that they are using a package that wasn't updated and might eventually stop working. I'd suggest the following:
All of this is applicable only to "unflagged" semver-major step and should have prior discussion and consensus with npm and packaging system maintainers and community in general, of course. These last questions are about how do we deal with phase 2 after flipping defaults. It's important to have general outline, but now I'm going back to the phase 1 which is optional flag that doesn't break the ecosystem. After considering all of the above, I agree that this default flipping plan is feasible and on the long run more desirable than what was proposed in this PR originally. I'm glad to see actionable way to proceed, and hope we can move forward with it. Hence, I plan to:
The discussion would be also simplified, as we won't have to have URLs, module types and shebangs tangled together. :) Does this plan sound good? |
That doesn't sound ideal, and would probably have security implications. According to https://webassembly.github.io/spec/core/binary/modules.html#binary-module, WASM modules must start with a magic string (
Historically, Regarding the whole plan, I'm not sure where I stand. It seems to me that the only POSIX compliant way forward would be to have a separate binary name, and if we introduce a non-POSIX way that happens to work for some wide-spread OSs, it will very likely start to be used on popular npm packages (either because their authors are not aware or they don't care), putting our users in an awkward situation. The status quo (either symlink your extensionless executable to a |
Just did a quick check but I don't see any comments on all the existing
code spawning child_process here. Moving from error condition to a valid
one worries me far less than swapping a default from one valid condition to
another without a clear way to migrate code like things spawning processes
(both from node and other envs).
…On Fri, Sep 1, 2023, 5:51 AM Antoine du Hamel ***@***.***> wrote:
5. We should add fallback to parsing entry point as Wasm if loading as ESM
fails
That doesn't sound ideal, and would probably have security implications.
According to
https://webassembly.github.io/spec/core/binary/modules.html#binary-module,
WASM modules must start with a magic string (0x00 0x61 0x73 0x6D), in all
likelyhood that's what we should use to decide if a file is ES or WASM.
Also, what about unknown non-empty extensions? Should there be any
difference in running myfile or myfile.exe or myfile.jabbascript?
Historically, node would parse anything as CJS unless for .json, .node.
When introducing "type":"module", it was decided we should take the
opportunity to restrict what extension was allowed in case we want to add
support for it later – e.g. do we really want to parse file.ts as ESM?
Wouldn't that close the door to support TS out-of-the-box in a later
version?
------------------------------
Regarding the whole plan, I'm not sure where I stand. It seems to me that
the only POSIX compliant way forward would be to have a separate binary
name, and if we introduce a non-POSIX way that happens to work for some
wide-spread OSs, it will very likely start to be used on popular npm
packages (either because their authors are not aware or they don't care),
putting our users in an awkward situation. The status quo (either symlink
your extensionless executable to a .mjs file, or use CJS) is portable,
and when it errors out it produces relatively easy to understand error
message; for most folks, it's an OK tradeoff.
—
Reply to this email directly, view it on GitHub
<#49295 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABZJI7ITJNZW65MQHZFGF3XYG4Z3ANCNFSM6AAAAAA33IPFKQ>
.
You are receiving this because you are on a team that was mentioned.Message
ID: ***@***.***>
|
Tbh as someone who is using But why can't we just have both options available, command line flag and named binary?
Maybe it would be worth adding to
tsc-agenda
|
There isn’t consensus, but this very well might become one. I think this is one of the main sticking points to figure out, and deserves its own issue. My recollection of #31415 was that it was done because we wanted to preserve an option for extensionless Wasm in the future, and we didn’t like the
Maybe there are other approaches. This should probably get its own discussion, because if we can’t reach a consensus on this aspect, there’s not much point in creating the new mode since one of its primary use cases is to enable extensionless ESM scripts.
I think it’s much simpler: we just add a flag which flips as many defaults as possible to ESM. Once that ships, we can possibly add the binary as an alternate way of running Node in this mode. And at some point, in a semver-major change we make the new mode the default and provide a way to opt into the CommonJS-default mode. We should design things such that it’s not awkward to flip back. I don’t think we need a collection of new flags for various smaller pieces of this. I wouldn’t block such efforts, but I feel like it would be harder to get consensus on
Yes, this was my intent. The current algorithm searches up the current volume to the root, and if it never finds a
I don’t think so. Changing how typeless package scopes are interpreted and discouraging typeless packages go hand-in-hand. There’s no point in having We don’t need to work out the question of how to patch CommonJS dependencies just yet. Whether it’s something at the npm registry level, something at the package manager level or handled by end users directly via a shell command or some script like
I wouldn’t bother doing I would just do one PR that includes everything, unless that’s too big to handle in one piece (in which case we can use a build flag to hide things until they’re all in place, or land them on main but mark them as “don’t backport” to prevent releasing until they’re all ready). Before we start, though, I think we should reach consensus on the open questions. I’ll open two issues:
There are also questions about how to migrate the ecosystem (adding The other thing we need is a list of concrete use cases, and not solutions presented as use cases. As in, “I want to be able to launch Node with an URL entry point” isn’t a use case; why do you want to do that? Whereas something like “I want to write what are typically called shell scripts in ESM JavaScript, where I can have a single file anywhere on my disk that uses ESM syntax and it doesn’t need a nearby |
@jirutka Do you mind giving me some examples of where flags in shebang lines do and don’t work? With the most popular/prominent platforms included. I’m trying to write documentation along the lines of:
In particular, we should include macOS and Windows (Windows Subsystem for Linux?) in this sentence, whichever list they fall into, as those two probably account for the majority of our users. For Linux, at least Debian and Alpine, as those are the two used in the official Docker images. |
I'm pretty sure dash and sh don't support them, as I had to account for that in nvm. |
@LiviaMedeiros Is there a reason to keep this open? I think #49869 covers everything except the URL stuff, and #49975 is in progress for that. |
This adds an option to directly load ESM entry point by absolute or relative URL.
Allows:
Doesn't allow:
I.e. most importantly, it allows to
search
andhash
Without breaking changes to how
node path/to/file.js
works.Might be related to: #34049
Fixes: #46009
Fixes: #49204
I'm not sure if this is the most correct way to implement it (or if a better alternative is planned), hence draft.
cc @nodejs/modules