-
Notifications
You must be signed in to change notification settings - Fork 43
Proposal: --default-type #335
Comments
This would be highly dangerous and break a lot of code by parsing with the wrong goal. The default is CommonJS, and back compat means that can’t change if, and until after, the community largely settles on a different default. It should not be possible to silently parse third-party code with any parse goals that the author didn’t explicitly choose - and the lack of an explicit signal definitively means CommonJS now, and for the foreseeable future. |
We do need to start thinking about what the upgrade paths will be in shifting the default to modules, and I'd hope we can have a clear guide and proposal on what users can expect in the long term here. I very much support having a proposal like this, and possibly even an implementation in order to allow those who are interested to start exploring these workflows. Ideally we should start coming up with recommendations for this process as well. For example, I chatted briefly with @arcanis (Yarn maintainer), about if it might be suitable to be injecting a "type": "commonjs" on install into node_modules already now, as that is the kind of work that would allow us to flip the default as in this proposal in due course, without breaking code. It is somewhat of a chicken and egg situation, but please lets get this discussion going. |
Maybe a better intermediate flag would be "--require-package-type" which throws when an explicit package type hasn't been provided? |
i don't want to ship a system where it needs some weird script that recursively patches already working code to run.
Even if the default system is esm, an extensionless file could be json or css or html etc, so this still wouldn't really be feasible. |
|
I really think this flag will eventually come around, but would like to see
less hand waving around the different data points and how they could be
configured. Namely, how we could configure things as @ljharb and @devsnek
point out to other values.
We have a set of configurable data that this flag would potentially be
affecting:
* STDIN
* ARGV (--eval , --print [print doesn't make sense for some module types])
* files without metadata on their format
* no package.json
* and/or, no extensions
We have a set of potential hard coded values (for now):
* commonjs
* module
We could expand the hard coded values as we grow the runtime
(--experimental-wasm-modules), or support other languages like TypeScript:
* `wasm`
* `{"": "typescript", ".ts": "typescript"} // bikeshed`
Or we could iron out the configuration of more complex configuration.
I think `{"type": "wasm"}` in the package.json is acceptable but somewhat a
bad choice as it does not hint at what happens to JS and if it stays CJS
for .js or becomes a JS Module which people might be looking for (I would
actually be hesitant to even set what .js does in this mapping because of
that).
I think in order to support other languages we have to agree on allowing
more complex configuration than a hardcoded list of values.
All of that however, is not blocking the nature of this issue. We can add
configuration complexity elsewhere and I would like to see that in
particular because it does not just affect `--default-type`.
I do like the flag to warn or error on importing something that lacks a
type field.
However, I do not think we should warn or error by default for a few
reasons:
1. Existing codebases do not have a "type" field and should continue to
work generally. The reason for breaking them would be something that
largely could be automated by project tooling/linting. Changing how
existing code works is not something I would see as a good thing.
2. You can `import()` things lazily (I even do this to speed up some CLI's
I work on) meaning it could be conditional or late in the app lifecycle.
3. You can `import()` things for debugging like via devtools, which ideally
shouldn't crash the app.
If we enforced requiring "type" when using `import` on initial shipping I
don't think that list is a problematic; otherwise we would move a non-error
condition to an error condition, which is a cause for concern to me.
…On Fri, May 31, 2019 at 5:20 PM Wesley Wigham ***@***.***> wrote:
I very much support having a proposal like this
? <#318 (comment)>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#335?email_source=notifications&email_token=AABZJI4I6WFWHJWS5HX2ZPDPYGQEJA5CNFSM4HRTEQ4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWWQGMI#issuecomment-497877809>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABZJI6UOHFTLAXSVJFBUT3PYGQEJANCNFSM4HRTEQ4A>
.
|
Sorry I didn't get the bandwidth to jump in before! As we discussed I have a lot of concerns about doing something like this, as it would imo be a relatively flaky solution: it wouldn't work well with PnP (because then we'd have to modify the files from the cache), it wouldn't work for node_modules that already got created (due to cache systems), it would cause a change of behavior from one day to the next, ...
I would tend to agree with this statement. Imo we should always consider "no type field" to be CommonJS, and the package manager shouldn't take responsibility in deciding otherwise. If this kind of logic was to be implemented, then the package registry would imo be the right place, as they're free to decide whatever rule they want to implement as to what is valid and what isn't. It would be possible for the npm folks to enforce that all packages published as of now must have a valid |
Can you expand on this a bit? I'm not sure I follow this logic.
This is exactly why we need to start doing this now to prep for the 5 year plan. Otherwise it could be more like a 10 year transition.
I agree, but we have to be practical here. If npm would implement this (and every other registry), then great. But otherwise how would we go about this? Or shall we just let the 10 year transition play out on its own here? |
A smooth long transition is much better than a shorter rougher one, imo. If everyone moving to ESM requires a push, then consider that it may not be the best end state (i believe everyone will and it is; but only with a smooth unforced migration path) |
Any sort of bytecode caches or snapshotting would be affected, consider:
If we compile it as CJS and cache the compiled form (using things like vm.Script#createCacheData) we get a specific buffer back that we can use to speed up bootstrapping. If we swap the translator used to be ESM:
I personally think we should expose capabilities to handle this as other global flags like |
Conceptually, all packages belong to the same cache that is shared by all the projects. They aren't copied to the
It also goes both way: under the current circumstances this patch approach would have to be implemented by everyone for it to be possible. Otherwise Node would have to keep the default to commonjs anyway.
If the issue here is "how to make The main concern would be with workspaces (since you'd then have to replicate the field everywhere), but since we're introducing constraints it would just be a |
I do not think node runtime flags should live in package.json and think
that is a completely different issue.
…On Tue, Jun 11, 2019, 10:36 AM Maël Nison ***@***.***> wrote:
it wouldn't work well with PnP (because then we'd have to modify the files
from the cache)
Can you expand on this a bit? I'm not sure I follow this logic.
Conceptually, all packages belong to the same cache that is shared by all
the projects. They aren't copied to the node_modules directory anymore.
As such, we can't just replace the content from the package.json within
the project node_modules - the only place where we can do this is inside
the cache itself.
If npm would implement this (and every other registry), then great. But
otherwise how would we go about this?
It also goes both way: under the current circumstances this patch approach
would have to be implemented by *everyone* for it to be possible.
Otherwise Node would have to keep the default to commonjs anyway.
I personally think we should expose capabilities to handle this as other
global flags like --preserve-symlinks can have similar effects in this
area now with package.json boundary scoped settings causing the mutations
to be non-trivial.
If the issue here is "how to make --default-type affect the main package
but not others", that seems like a reasonable compromise? Especially since
it would be quite difficult to ask users to add --default-type to all
their JS scripts, but adding a configuration settings in the package.json
is reasonable enough. It would have more design space as well, such as
opening the way to support Node configuration within the package.json
file (which will be required for loaders anyway).
The main concern would be with workspaces (since you'd then have to
replicate the field everywhere), but since we're introducing constraints
<https://yarnpkg.github.io/berry/features/constraints> it would just be a
fix away 🤷♀️
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#335?email_source=notifications&email_token=AABZJI4HQBOKPKWI3U23CALPZ7BAVA5CNFSM4HRTEQ4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXNRQUQ#issuecomment-500897874>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABZJI2FM5LWJ76EG3HNR4TPZ7BAVANCNFSM4HRTEQ4A>
.
|
That’s what type module already does in package.json, no? |
If we can get back to the original feature request, it was for a user to flip Node’s defaults to be ESM first rather than CommonJS first. The question is what that means in practice. As I wrote at the top, if we don’t want to confront the complication caused by extending this to dependencies, we can just provide an option that limits the scope to |
Just tested
And it works. So really the proposed The only other case I can think of is non- |
This is a proposal to provide a way to change Node’s default module system from CommonJS to ESM (#318).
There are two places where the module system can be specified, with a lack of specificity defaulting to CommonJS:
package.json
"type"
field, where the lack of the field is interpreted as"commonjs"
.--input-type
, where the lack of the flag is interpreted ascommonjs
.We propose a new flag
--default-type
, to control the default values ofpackage.json
"type"
and--input-type
:--default-type=commonjs
is the same as the current behavior, where the lack of the flag or field is interpreted as CommonJS.--default-type=module
would cause the lack of the flag or field to be interpreted as ESM.The expectation is that users who prefer ESM to be their default would set this flag in their
NODE_OPTIONS
, e.g.NODE_OPTIONS=--default-type=module
.With the flag set to
module
, some potential use cases are:Commands like
node --eval 'import { version } from "process"; console.log(version)'
could be run without needing--input-type=module
.Extensionless files to be run like shell scripts could use ESM syntax, without needing to be symlinks to
.mjs
files or use other workarounds.Under
--default-type=module
, most typical projects with dozens of CommonJS dependencies would break. Most, if not all, of those dependencies would lack"type": "commonjs"
in theirpackage.json
files, at least for now while public awareness of the new field grows. One potential solution is a script that users could run to add the field for any dependencies that lack it:We expect that package managers might soon begin to add
"type": "commonjs"
automatically for dependencies that lack a"type"
field. If they don’t, and no other userland solution finds broad acceptance, another solution is for Node to add a third option to--default-type
, namely--default-type=module-except-dependencies
. This would behave the same as--default-type=module
except that it wouldn’t apply to folders under anode_modules
folder. We would prefer not to need to implement this, however, as a “pure” solution where package managers or userland utilities bridge the gap would let Node provide a more ideal API. This option can always be added later if it turns out to be necessary.The text was updated successfully, but these errors were encountered: