Skip to content
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

Detect require vs. import based on the entry point, not package.json #39353

Closed
Pomax opened this issue Jul 11, 2021 · 45 comments
Closed

Detect require vs. import based on the entry point, not package.json #39353

Pomax opened this issue Jul 11, 2021 · 45 comments
Labels
feature request Issues that request new features to be added to Node.js. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. module Issues and PRs related to the module subsystem. stale

Comments

@Pomax
Copy link

Pomax commented Jul 11, 2021

Is your feature request related to a problem? Please describe.

Node does not require a package.json file to run, unless you're writing modern import/export code, in which case you suddenly need to define a package.json even if you have no intention of creating a project out of the code you just wrote. You can't even use node --input-type=module because it will --for no reason that makes sense for users-- complain that you Cannot use import statement outside a module, the thing we're literally saying that's what we're doing by using that flag.

Describe the solution you'd like

Node should not need folks to tell it which parsing mode to use: it should scan the entry point it's being asked to run, and simply check whether or not the reserved JS keyword import is found in that file. If it is, done: run in ES module mode without needing folks to create a file that Node should not rely on to do its job.

  • node is invoked with a file as run target
  • node scans that files for the import keyword
  • if it's found, node runs the target in ES module mode
    • any use of require(...) during the run is now a perfectly normal "this function is not defined in this scope" error.
  • if it's not found, node runs the target in CJS mode
    • any instance of import during the run is now a perfectly normal "import is a reserved keyword" error.
    • any instance of the dynamic import(...) during the run is now a perfectly normal "this function is not defined in this scope" error.

And now Node does the correct thing, given perfectly normal code as run target. Will that very first step add to the startup time before code actually runs? Sure, but scanning for the import keyword takes on the order of nanoseconds, not milliseconds. We're not scanning the entire dependency tree to see if somewhere down the line we suddenly switch from import to require or vice versa: by default, without a package.json, Node doesn't need to mix and match: as default behaviour Node should run both legacy CJS and modern JS (either/or, not a mix, obviously) without runtime flags or needing files that have nothing to do with Node itself created.

Describe alternatives you've considered

There are no alternatives: Node should not rely on package.json just to run normal modern code, it should do the right thing automatically, with runtime flags and package.json only for folks who need it to do something else (and it should probably never need package.json, that file is so that NPM can do proper package management. Node should never need to consult it just to run normal, valid, modern or legacy, JS)

@aduh95
Copy link
Contributor

aduh95 commented Jul 11, 2021

Related discussion: #37857, nodejs/modules#300 (comment), nodejs/modules#296.

  • node scans that files for the import keyword

I don't thing that's a reasonable solution: it would introduce a performance penalty on startup time if node has to parse the file for import (dynamic import() is a completely valid thing in CJS, are you suggesting you should remove that? And we would need to know if the import keyword appear inside a string or a comment, so it would be pretty expensive to determine that), and also not all ES module contains import so that would introduce some weird edge cases (AFAIK, there is no known algorithm to tell for sure if a file is written as a module or a script).

There are no alternatives:

You could use loaders to customize which format / parse goal for a given file: https://nodejs.org/docs/latest/api/esm.html#esm_loaders. It's not stable yet though.

Node should never need to consult it just to run normal, valid, modern or legacy, JS

Legacy code is exactly why changing the default behaviour of Node.js is a very difficult thing to do, it would be a problem if older code base stopped working. Note that Node.js looks for package.json only for .js file, if you use .mjs or .cjs files Node.js won't be looking for any package.json.

@Pomax
Copy link
Author

Pomax commented Jul 12, 2021

dynamic import() is a completely valid thing in CJS, are you suggesting you should remove that

I'm suggesting changing the default behaviour, so if Node should allow mixed import/require when there's a package.json, then perfect: that is not at issue, I'm all for that. But Node should also do the right thing without a package.json.

it would introduce a performance penalty on startup time if node has to parse the file

Correct. And that would be very little time because it only has to scan one file, plus (and this is the big one): if you use a package.json, Node doesn't need to scan anything, it already knows which mode to run in, so it would do exactly what it's already doing right now, and no one following current necessary practices will be impacted. Only folks who invoke Node without a package.json would run into this performance hit, although I struggle to call it that: it'd be insignificant compared to the rest of Node's initialisation.

You could use loaders to customize which format / parse goal for a given file

While I see a lot of potential for loaders, they are antithetical to why I filed this issue: Node should do the "obvious" right thing out of the box (i.e. if asked to run a file, it should run that file, either in cjs mode, or esm mode, automatically. Not first be configured to do the right thing through runtime flags, or config files, or config code).

Note that Node.js looks for package.json only for .js file, if you use .mjs or .cjs files Node.js won't be looking for any package.json

Sure, but again: that's not doing the right thing, that's forcing people to use nonstandard extensions as workaround. Node should (and this is a long term "should", not "it must, now!") do the right thing when given a .js file, based on the content of that .js file (and also, only that .js file. There is no need to resolve the full tree, Node can do the "obvious" thing based on the fact that people who use mixed import/require will almost certainly be using a package.json already to make sure things work properly).

Legacy code is exactly why changing the default behaviour of Node.js is a very difficult thing to do

No it isn't? That's what major versions are for. I have no expectation of this getting changed in the current version, but I do kind of expect this to be seriously considered for Node 17 or 18. The way Node works as of v16, especially as LTS, should stay exactly what it is now, let's definitely not change that. People rely on it to work a specific way. But 17 or 18 are fair game.

@aduh95
Copy link
Contributor

aduh95 commented Jul 12, 2021

Hum so you'd like to see this behavior only for the entry point 🤔 In this case, I have more questions, how would you define parse goal for dependency of such modules:

// entry-point.js
import './submodule.js'; // is this ESM no matter its content?
import './submodule.cjs'; // is this CJS?

import 'some_package'; // can this be CJS?
// other-entry-point.js
require('./entry-point.js'); // is this CJS?
// another-entry-point.js

// no import, no export, no require, is this ESM or CJS?

console.log(this); // is this undefined?

var obj2 = { get x() { return 17; } };
obj2.x = 5; // should this throw?

I'm suggesting changing the default behaviour, so if Node should allow mixed import/require when there's a package.json, then perfect: that is not at issue, I'm all for that. But Node should also do the right thing without a package.json.

Why would the "right" thing to remove dynamic imports for CJS? Are they causing an issue, or is this just for convenience to tell if a file is CJS or MJS?

While I see a lot of potential for loaders, they are antithetical to why I filed this issue: Node should do the "obvious" right thing out of the box (i.e. if asked to run a file, it should run that file, either in cjs mode, or esm mode, automatically. Not first be configured to do the right thing through runtime flags, or config files, or config code).

Note that what is "obvious" to you may not be for everyone. While I understand loaders are not the long term solution you'd like, it can still be useful for you (or someone else) to build a POC.

@devsnek
Copy link
Member

devsnek commented Jul 12, 2021

this is not a novel topic of discussion and I think the general sentiment is that this would be cool if someone builds something, but no one has built something because it's a very complex problem, and you have to make a lot of subjective calls which people disagree on.

@Pomax
Copy link
Author

Pomax commented Jul 12, 2021

Good questions. In the absence of a package.json, keep it simple, and keep it naive:

// entry-point.js
import './submodule.js'; // is this ESM no matter its content?
import './submodule.cjs'; // is this CJS?

import 'some_package'; // can this be CJS?

would result in going "that's a normal import on line 1, stop checking and use ESM parsing" and then when it runs the file it tries to import submodule.cjs using ESM rules and it'll throw an error, and that's fine. Use a package.json if you need to mix modalities.

// other-entry-point.js
require('./entry-point.js'); // is this CJS?

Yes, it is, because the first import mechanism encountered uses require, not import.

// another-entry-point.js
// no import, no export, no require, is this ESM or CJS?
console.log(this); // is this undefined?

This is Node's default mode. If that's still CJS by v17 or v18, then it's CJS, unless Node's finally ready to switch to ESM parsing by default, then it's ESM.

Why would the "right" thing to remove dynamic imports for CJS? Are they causing an issue, or is this just for convenience to tell if a file is CJS or MJS?

Who said anything about removing dynamic imports? If the file starts as a normal, modern, plain, JS file (that is it starts with a bunch of imports of other files that end in .js) then it picks ESM parsing, and rolls with that, and if there are dynamic imports, no problem. Those work fine. If it starts with a bunch of requires then pick CJS parsing, and roll with that instead, and then if there are dynamic imports that are valid under the CJS model, then also no problem.

This feature request is entirely about what Node does when asked to run "plain files", in the absence of a package.json. That is, we're not dealing with a project, we're not dealing with external dependencies, someone just wrote a bunch of plain JS files using Node's API and they want to run them, they just happen to be more than a single file because the person doing the writing was taught proper code house keeping and sticks to that. Node should be able to run their code without anything other than "being run": in this scenario, Node can pick the correct parsing mode 100% of the time in these cases, because the examples you're showing just don't factor into that use-case.

This really is a matter of "if the entry file starts as a normal JS file with import, use ESM, if it starts with require, use CJS, and we're done". Any "oh hey you actually did something unusual" code like in those examples should, quite reasonably, halt with an error telling folks that they're mix-and-matching, and to use a package.json (or a runtime flag that explicitly tells Node which parser to use, which would most certainly be worth adding to 16, as that would be purely new functionality and not break any preexisting code).

Note that what is "obvious" to you may not be for everyone.

Hence the quotes, I'm talking about the kind of obvious that exists when looking in from the outside: "I have some JS files, I've written them following current standards, running node firstfile.js should works". The kind of expectation that someone who is new to Node would have.

@aduh95
Copy link
Contributor

aduh95 commented Jul 12, 2021

Thanks for the clarification, I think this makes a lot of sense; first we'd need to find or write an algorithm that can tell if a JS file is ESM or CJS, is this something you'd be interested in contributing?

Related (vaguely): TC39 proposal "use module";.

in the absence of a package.json [...], we're not dealing with a project, we're not dealing with external dependencies

If you allow me to be pedantic, you can have external deps without a package.json – Node.js doesn't use package.json to resolve local deps and there is an open PR to add support for HTTPS imports :)

@targos
Copy link
Member

targos commented Jul 12, 2021

we'd need to find or write an algorithm that can tell if a JS file is ESM or CJS

Something that goes in this direction is being done in #39175

@Ayase-252 Ayase-252 added feature request Issues that request new features to be added to Node.js. module Issues and PRs related to the module subsystem. labels Jul 13, 2021
@Pomax
Copy link
Author

Pomax commented Jul 14, 2021

If you allow me to be pedantic, you can have external deps without a package.json

Ah, yes that's true, good point.

Something that goes in this direction is being done in #39175

Nice!

we'd need to find or write an algorithm that can tell if a JS file is ESM or CJS, is this something you'd be interested in contributing?

While I wouldn't mind giving it a shot, given the number of projects I'm already involved with any promise to try to get to that soon would pretty much be a lie, I'm basically booked up for projects for at least a year, filing issues and discussing whether there's merit to it is the most I can do for larger projects for the foreseeable future =(

@aduh95
Copy link
Contributor

aduh95 commented Aug 4, 2021

#39508 didn't get much traction, and there was at least two outstanding objections for not implementing this proposal.

@Pomax
Copy link
Author

Pomax commented Aug 4, 2021

These arguments seem to apply on the idea that "this makes it easier for beginners". That's an argument I don't buy into. Changing the behaviour as suggested in this issue (making Node use ESM parsing mode if the entry file and only the entry file is ESM) makes Node better as a tool. Tools should have sensible default behaviours, and a way to override any and all of that behaviour by specifying an explicit configuration (either throught runtime flags, or a config file, which for Node is the package.json file).

So if we're talking about sensible default behaviour: if someone writes plain, modern JS, which because of the JS spec can only be ES module based code (because that's what TC39 decided is the only import mechanism that spec-compliant JS can use) then Node should be able to execute that file, because Node's job is to run JS, which means at the very least it should run spec-compliant JS, even if it can also run its own flavour of JS. And if it's asked to run legacy CJS code, it should run that without any complains too, because it's been doing that for about a decade now; that should keep working. And if someone writes code and specifies a the configuration file that Node looks at, then obviously now Node should do whatever it does based on what's in that configuration file. This is basic tool design: if there's a config, that config kicks in.

And yes: that means that ESM code that works "on its own" suddenly won't run anymore when you use npm init to create a package.json file, because npm right now does not include the type field. That's npm's failing (and if there isn't an issue for this on their side already, I will be quite surprised), not Node's. In fact, that's exactly what you want: there is no problem here, Node will tell you that it can't run ESM unless you add "type": "module" to your package file, you follow Node's instruction and update package.json, and immediately your code works again.

"What if your ESM code has imports that somewhere down the line switchi to CJS style requires?"
Literally everyone doing that today already has a package.json file, because their code won't run if they don't. This is a non-issue, their code keeps working exactly as it already works right now.

"What if someone mixes ESM and CJS in their own (flat file) code?"
Then someone should tell them to fix that, because that's just nonsense. And that someone is the Node cli when it's asked to execute a script. It'll throw you an error right now if you try this, and it should keep throwing you that error.

"What if their ESM imports a Node API, which uses CJS?"
This would be the only valid argument against, if it weren't for the fact that Node can already expose its own API in ESM context just fine, making this a non-issue, too.

@bmeck
Copy link
Member

bmeck commented Aug 17, 2021

CC: @nodejs/modules

I think we should close this as a duplicate of many older issues and discussions unless something novel comes up.

Per:

Something that goes in this direction is being done in #39175

This is only a heuristic to help debugging, it isn't 100% reliable or performant nor is it expected to become so to my knowledge.

@Pomax
Copy link
Author

Pomax commented Aug 17, 2021

The hope was that this explanation was the "novel" thing people keep mentioning. Every other issue seems to want way more than what Node, the command line util, should be doing, making this proposal far more reasonable in comparison.

@bmeck
Copy link
Member

bmeck commented Aug 17, 2021

I don't see any new arguments here to be clear. I think something novel would need to explain how to avoid breaking things rather than stating that breaking things such as existing code without package.json (the scope of the desired feature) isn't a problem.

@guybedford
Copy link
Contributor

If anyone is interested in working on this problem, I can provide the guidance wrt the right contributions to cjs-module-lexer to enable this analysis to be fast. Feel free to DM me. I don't personally have the bandwidth to take such a thing on as much as I would like to.

@devsnek devsnek closed this as completed Aug 18, 2021
@Pomax
Copy link
Author

Pomax commented Aug 18, 2021

Nor do I, given both my day job and other FOSS projects I'm involved in, but closing it feels the wrong solution. Keeping it open with a "contributors welcome" label would seem a better approach here.

@aduh95 aduh95 reopened this Aug 18, 2021
@aduh95 aduh95 added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Aug 18, 2021
@bmeck
Copy link
Member

bmeck commented Aug 18, 2021

To be clear, even if parsing is very fast I don't think we should land the currently proposed feature in this issue. See previous PR linked above and Issues/PRs before that. I will state in 2014 I was a proponent of dual parsing, but would rather we find an approach without grammar collisions and that is forwards compatible.

@Pomax
Copy link
Author

Pomax commented Aug 18, 2021

I'd like to counter that you can do both: you have grammar collision right now, moving to a solution that works without solving that, and then moving to a solution that does solve that afterwards, is infinitely better than skipping the step in between and having people wait years more before node "just works" when asked to run a standards-compliant file. (After all, the more time passes, the more the TC39 syntax is the correct grammar that interpreters should always support, even if they also support some other non-standard grammar, and the more cjs becomes "that thing that we needed in the past, but why is it still the thing node insists on is the standard?")

@bmeck
Copy link
Member

bmeck commented Aug 18, 2021

@Pomax the grammar collisions have increased over time, not decreased.

@ljharb
Copy link
Member

ljharb commented Aug 18, 2021

@Pomax there are two standard TC39 syntaxes, not one, that can not be unambiguously distinguished by parsing, and at this point that will never change.

@guybedford
Copy link
Contributor

@Pomax the ambiguity that is an issue is a file without any import or export defaulting to being treated as CommonJS when that is still a valid module. Eg features like top-level await not being available for scripts unless you add a dummy export {} to trick the parser which then becomes a reified hack.

@jkrems
Copy link
Contributor

jkrems commented Aug 18, 2021

Other examples of subtle differences:

  • import.meta may only appear in modules and would be a syntax error in scripts or CommonJS.
  • const async = 42 at the top-level may not appear in modules but is valid in scripts or CommonJS.
  • Anything affected by strict vs loose mode.
  • this is undefined in a module but the exports object in CommonJS and (by default) globalThis in a script.

Some of these can be detected by parsing - and some cannot. And even for the ones that can be detected by parsing: What if a file contains:

const async = 42;
export {}

Is this an invalid script or an invalid module? It's an early/parse error. But in which line?

You can gloss over these by saying "every module has to have at least on static export or import statement and every file with at least one of those is a module" and then interpret the contents based on those assumptions. But that's not the semantics of JavaScript as it's standardized, it's only a subset of JavaScript. JavaScript allows a module that just uses import() expressions and still has module semantics, not script semantics.

@Pomax
Copy link
Author

Pomax commented Aug 18, 2021

In these cases, I'd honestly strongly advise to go with "whatever lets node script.js run without error the majority of the times, and throw a sane error when it can't". These are all great examples of letting the perfect be the enemy of the good: node doesn't need to "succeed on all possible input". That was never the request in this issue either (it would be a pretty crazy request): have Node keep failing if it can't tell what it needs to do, but improve node's ability to tell which mode it needs to run in so that it fails less than it does right now.

If the entry file has import or export, we know it's going to need to be parsed as ESM, the end. Node might still error out because of grammar incompatabilities: that's fine. In those cases, it's the user's responsibility to help Node figure out what to do and there are mechanisms for that. However, we've now also drastically reduced the number of cases in which Node will error during invocation.

The goal should not be to make Node do the right thing on every input and every edge case: all it needs to do is "just work" more often than it does right now, in cases where it's obvious which mode it should be running in. On that note, if there are no import/export then indeed, we don't know if this is a plain script or CJS: this too is fine, that's where we are right now, and what Node does right now in those cases is perfectly acceptable behaviour.

Node can be made to fail less.

@ljharb
Copy link
Member

ljharb commented Aug 18, 2021

imo node.js definitely needs to succeed (syntactically) on all possible JS input.

It's not just a problem with the momentary snapshot of a file - which may contain ESM syntax, or CJS syntax, or be ambiguous. It's also that when you start to make your file, before you add any import or export statements, it'd be parsed as CJS - and when you add those things and test again the next day, it'd be parsed as ESM. That's confusing and doesn't help users. That's node trying to guess the intent of its users and being dangerously wrong.

The user's responsibility is to use the extension and package.json to tell node what parse goal is intended, and then node can always parse it.

@devsnek
Copy link
Member

devsnek commented Aug 18, 2021

honestly i still don't understand why people are so adverse to using .mjs

@bmeck
Copy link
Member

bmeck commented Aug 18, 2021

@Pomax I think a more useful line of discussion would be to work out the exact nature of the feature you want rather than trying to state that you want things to act in a way that obviously has contention and a lot of history.

If the goal is only to discern file contents as a heuristic (not a standard supported discernment) https://github.com/tc39/proposal-modules-pragma for example covers the ability to statically declare that a source text should be in a specific mode of JS (both Script and Module are first class, neither deprecated) and avoids a grammar collision so that could be a more amicable route. However, this goal is odd since it starts to expand into other things, like should WASM do a similar heuristic, or TS if Node supports that as a first class module syntax for example. The parsing method is just too forwards compatibility problematic and doesn't serve to allow all Modules writing in the JS spec to run under this method. It also means that adding a package.json above the file would further alter how the file works. This is a lot to discover vs a set of small rules even if that means action must be taken by a developer to resolve the error.

However, if the goal is just to run the entry point in a special manner as above it means that behavior alters based upon if the module is the entry point. With various runners like PM2 or even things like AWS lambda to some extent, there is a long history of the user application not being the entry point for the node process. A solution likely needs to mitigate this, and likely would have to apply to all files in some way, not just the entry point.

@guybedford
Copy link
Contributor

@bmeck with eg runners, surely they still use spawn or fork, so any flag or top-level behaviour carries over fine to these scenarios? It's the sort of argument that can easily discourage further discussion unless clearly unpacked.

@bmeck
Copy link
Member

bmeck commented Aug 18, 2021

@guybedford I know that at least FAAS are keeping hot instances by doing something during startup, so they are not using spawn/fork. I am fine having a discussion but not convinced that the idea of parsing simplifies things like I was at one point in time. I think parsing leaves a very precarious edge that we don't have a guarantee of being safe that I am trying to explain. I also think having unique behavior for entry points likewise has an edge. Wrappers around ESM do exist and even in some workflows export * from 'actual-location'; is used purely for aliasing purposes and those too would suffer problems with this. Attempting to state that a single source of problems is the only source seems unlikely to me; I am much more interested in gaining consensus by agreement that one of the following is the path forward:

  • that a feature does have problems and stating the benefits are great enough to overcome these but documenting the problems and considering forwards compatibility problems around them; this seems the current course of discussion only so far but doesn't seem to be agreed upon
  • mitigating the problem, this is what I would think as the next approach of trying to get the feature in if there is agreement that there are problems but requires agreement on what needs to be mitigated. Often mitigation requires opt-in but this proposal in this issue doesn't use any opt-in except being an entry point. This attempt at mitigation seems to actually cause other problems, so a different one would likely be the next approach such as a different method that can be subject to static analysis.
  • agreeing that the problem is too costly for its benefits. I think all previous attempts to solve for this have ended up here since it would be the default state of things if the other two paths cannot be agreed upon.

@GeoffreyBooth
Copy link
Member

This was implemented in nodejs/ecmascript-modules#55. That PR forced the user to use a new flag, --entry-type=auto (which would now be --input-type=auto since we renamed --entry-type). The requirement of the flag made the change backward compatible. See also the PRs linked from that one.

@Pomax
Copy link
Author

Pomax commented Aug 19, 2021

@GeoffreyBooth that sounds amazing, and it would be super useful if its docs specified which values it can take, rather than leaving users guessing (it currently says --input-type=... set module type for string input).

Still, the fact that it's there means that most of this issue's proposal might be entirely moot. However, I just gave this a try and as far as I can tell, this doesn't seem to do what what this issue is about:

Using a test.js that just contains import { fs } from "fs"; console.log(fs);, and running as module, I get this:

> node test.js --input-type=module
(node:1796) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
(Use `node --trace-warnings ...` to show where the warning was created)
D:\test.js:1
import fs from "fs";
^^^^^^

SyntaxError: Cannot use import statement outside a module

And if I have a dir with a package.json that indicates type module, and I try to run a CJS script that just contains const fs = require("fs"); and run it as commonjs input, that also fails:

> node test.js --input-type=commonjs
file:///D:/test.js:1
const fs = require('fs');
           ^

ReferenceError: require is not defined in ES module scope, you can use import instead
This file is being treated as an ES module because it has a '.js' file extension and 'C:\Users\Mike\Documents\Git\projects\js-datastore-with-schema\package.json' contains "type": "module". To treat it as a CommonJS script, rename it to use the '.cjs' file extension.

@ljharb
Copy link
Member

ljharb commented Aug 19, 2021

input-type controls input - eval'd code or piped-in code - not files.

@Pomax
Copy link
Author

Pomax commented Aug 19, 2021

Cool: so where's the option too force it for files, too? Because that would solve this entire issue. And if there isn't one: can we add one to solve this issue? A runtime flag is 99.99% as good as "doing the right thing without a runtime flag".

@ljharb
Copy link
Member

ljharb commented Aug 19, 2021

There is no option, by design. The only way a file's parse goal must ever be determined is by extension + package.json.

@Pomax
Copy link
Author

Pomax commented Aug 20, 2021

But that's bad design for a commandline utility. The whole point of runtime flags is that people don't need entire configuration files to specify individual primitive (e.g. number or string) values. node whatever.js should do whatever it does out of the box, unless an env var is set (e.g. NODE_ENV), in which case that will override that aspect of the default behaviour. Unless, in turn, there's a configuration file (i.e. package.json) with overrides on node's default behaviour, in which that should win. Unless a runtime flag is provided, which is the highest authority when it comes to now a CLI utility runs.

Forcing this though an external config only is cli design can be made so much better.

(Sure, "where does it stop?" because everyone's afraid of the slippery slope, but in this case that's a fairly straight forward answer: it stops when people stop asking for individual runtime flags to mirror individual primitive values that are currently only available through a separate package.json file. Not allowing flags for complex values: perfectly sensible. Not allowing runtime flags for stringsand numbers that change how node runs: that's just ... strange)

@ljharb
Copy link
Member

ljharb commented Aug 20, 2021

nodr isn’t solely a command line utility - it’s a programming platform.

A textual file is already a primitive that encapsulates its parse goal by extension. That you can use package.json to subvert that doesn’t mean that all possible means of subversion should be permitted.

@Pomax
Copy link
Author

Pomax commented Aug 20, 2021

Yes, but node, the CLI, is a command line utility. This has only ever been about that aspect of Node. "invoking node, the executable, so that it does the right thing more often than now, without adversely affecting any existing codebase".

And no, a text file isn't a primitive, it's an external dependency in this case, with additional cognitive load, and a clunky user experience for things that could easily be runtime flags.

Heck, to draw parallels to other programming tools: this is akin to passing -std=c90 or -std=c11 to gcc: it is such an important part of runtime control (because it changes how the tool parses grammars) that that should be the very justification for including the same control for node (the cli command), which also has to deal with multiple grammars. Not the justification for excluding it.

@GeoffreyBooth
Copy link
Member

@GeoffreyBooth that sounds amazing, and it would be super useful if its docs specified which values it can take, rather than leaving users guessing (it currently says --input-type=... set module type for string input).

The PR was never merged in. I’m just referencing it since it seems to do what you’re asking for. It would need to be updated to reflect changes to the ESM implementation since it was written, most prominently that a new flag would need to be chosen. Also presumably the version of Acorn in Node core is newer now, so the problem with import() would be avoided.

@kasperk81
Copy link

With ESM gaining traction in and beyond node.js worlds, I am now also in the category of users who find both the available options to enable ESM less-than-ideal: .mjs extension or package.json. They just come across as superfluous and unnatural requirements.

Two questions:

  1. Is breaking change an option for the next (or future) major versions of node?
  2. For sake of argument, if breaking change was an option, what would be the ideal no-compromise performance-wise breaking change here, that will lead user to the pit of success -- without requiring .mjs extension or existence of package.json?
    • In the absence of .mjs and package.json explicitly specifying
      how to behave ...
      Treat `require()` as CJS and `import` as ESM. If the user is
      assuming something different, let things continue (and fail)
      implicitly.
      
      Improve debugging/analysis tools to spot potential code issues during the development time.
      
    • something else?

@bmeck
Copy link
Member

bmeck commented Oct 4, 2021

Is breaking change an option for the next (or future) major versions of node?

It is unlikely that a change based upon syntax guessing will make it in; but breaking changes do happen for more minor things that are more forwards compatibility safe after the breakage, yes.

For sake of argument, if breaking change was an option, what would be the ideal no-compromise performance-wise breaking change here, that will lead user to the pit of success -- without requiring .mjs extension or existence of package.json?

Node could always just ship a 2nd binary that swaps the default. A node_esm executable or some similar name for example. Such an alternative binary wouldn't break code when calling out to child_process/the other concerns presumably since code would be written against it and the concerns about altering behavior accidentally wouldn't come into play since the behavior is reliably the same even if CJS, ESM, or a new parse goal collide further than they do today.

@ljharb
Copy link
Member

ljharb commented Oct 4, 2021

I don't understand why .mjs feels "unnatural" - does using .js feel unnatural? You already can't just use any arbitrary file extension you want without extra configuration - does anything on the server (not just in the JS world) work that way for code?

@kasperk81
Copy link

It is unlikely that a change based upon syntax guessing will make it in;

Yes, my thinking is also aligned that such validation can/should be done by the code analysis tool rather than the node.js runtime. In this situation, I'd rather have the runtime continue and fail in implicit ways (whatever may happen, happens), rather than guessing anything just to be able to produce a high-level speculative-at-best kind of error message.

I don't understand why .mjs feels "unnatural"

If this ES feature "just works" with .js extension like many other ES features do, then there is no confusion and we don't have to explain anything. When it doesn't work OOTB, that kicks things into the situation where we are right now.

does anything on the server (not just in the JS world) work that way for code?

No, but you don't ask users to change file extensions to use another feature of same language, just because it conflicts with an existing feature; a command-line option (which we are missing here) and configuration option (which we have in package.json) comes to mind; sometimes accompanied by additional environment variable support. This approach of "for this particular feature only, change extension of your files" is a bad design and should not be promoted for anything, imo.

@ljharb
Copy link
Member

ljharb commented Oct 4, 2021

@kasperk81 that's not really possible though, because the JS spec created two ambiguous parse goals. Anything that appears to "just work" will also silently fail for nonzero programs.

JS kind of contains two languages now - ie, two parse goals. Modules and Scripts aren't the same and it makes perfect sense to me that they require different extensions (or explicit configuration).

@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2022

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Apr 4, 2022
@github-actions
Copy link
Contributor

github-actions bot commented May 4, 2022

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@aduh95
Copy link
Contributor

aduh95 commented Oct 27, 2023

FWIW Node.js 21.1.0 ships a --experimental-detect-module CLI flag that will load an ambigeous .js file as ESM if it can't be parsed as CJS.

@Pomax
Copy link
Author

Pomax commented Oct 27, 2023

that's great to hear, thank you letting us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. module Issues and PRs related to the module subsystem. stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.