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

Support for ECMAScript Modules (ESM) #236

Closed
tunnckoCore opened this issue Jan 22, 2019 · 18 comments
Closed

Support for ECMAScript Modules (ESM) #236

tunnckoCore opened this issue Jan 22, 2019 · 18 comments
Labels
blocked enhancement New feature or request

Comments

@tunnckoCore
Copy link

tunnckoCore commented Jan 22, 2019

Continuation of few comments on #21.

Even that this is Node oriented, it still should understand module field. That's not against the intent of the project, the output is still node compatible because it is CJS. The module-first is only helping for smaller outputs of the ncc-ed package, because webpack does better tree shaking over ES Modules.

I tried. And it's definitely a thing. Consider the following.

Package foo and package bar. Both are written in ESM. Both have module field. We ncc the bar package (with --minify), which uses only a few things from the foo package. The output of the bar package, won't have the unused things from the foo package. If foo does not have the module field, the bar output includes everything from foo.

If not by default, at least support a flag option for this.

Reproduce: https://github.com/tunnckoCore/ncc-modules
Open the packages/bar/dist/index.js and you won't see the FOO_PKG because it isn't used in bar.

The only local change on node_modules on ncc is just removing the mainFields option. And still, the outputs are node compatible and CJS, so I don't see why not support this thing, by default with the Webpack's default resolving ['module', 'main']. Also, even if everything is written in CJS or package does not have module field Webpack will fallback to main anyway.

@rauchg
Copy link
Member

rauchg commented Jan 23, 2019

Node doesn't honor the module field, so I think ncc shouldn't. You'll get a different outcome by running node x and ncc run x

@tunnckoCore
Copy link
Author

Yea, agree. That's why through flag will be the best way. It won't break anything, but will allow above scenarios. And generally, it doesn't make sense to create a whole new tool just because few bytes change and some resolving thingy :)

@guybedford
Copy link
Contributor

FYI there are currently no proposals to support the "module" field for Node.js's modules implementation, and in fact in many cases it would be a breaking change for Node.js to introduce it so it seems likely Node.js will not support it (although anything could still happen).

@tunnckoCore
Copy link
Author

Agree. But still. Good tools should have flexibility. Such option won't break anything for ncc users and similar use cases. It's a great tool for bundling on top of another one.

I'm asking at least for a flag/option, which won't hurt anyone :)

@balupton
Copy link

balupton commented Apr 9, 2019

Our zeit now builds started failing last week due to ncc hitting the editions autoloader for various packages, which their main field points to the edition autoloader.

The editions autoloader is skipped by browserify etc, as they prefer the browser field, which points directly to the browser edition, skipping the autoloader.

I've been looking into options here for what I can do to rectify this, as Bevry's libraries/utilities are quite popular, with the editions autoloader getting millions of downloads a month.

Having ncc support the browser field would allow editioned packages at least some hope.

Other than that, does ncc prefer .mjs files over .js files? That could be our other option, as then the mjs file could just be an export * from './editions-esm/index.js', bypassing the edition autoloader.

Otherwise, I may need to look into making the editions autoloader a node.js require extension for package.json files, which would be a major usability hurdle.

The ideal of course would be if bundlers added first class support for the editions format, that way, ncc, as it supports typescript, could elect for the source edition for typescript projects.

@guybedford
Copy link
Contributor

@balupton perhaps there is a way you could design the autoloader to be ncc-compatible? For example using non-analyzable requires to ensure it builds the right way between ncc and Node native? I know its not pretty, but something like:

if (ncc) {
  require('./main.js');
}
else {
  eval(fs.readFileSync('./autoloader.js'))
}

sort of thing.

Unfortunately supporting the browser field would break a lot of browser builds for us.

For an .mjs main are you thinking of something like "main": "x" where the .mjs or .js version is selectively used depending on the environment support? It isn't yet clear whether this pattern will ever be supported for Node.js, so again, I'd be hesitant to follow a pattern that doesn't lie on the standard path. The current Node.js --experimental-modules implementation doesn't support "main": "x" resolving to x.mjs as exact extensions are required.

@balupton
Copy link

balupton commented Apr 9, 2019

For an .mjs main are you thinking of something like "main": "x" where the .mjs or .js version is selectively used depending on the environment support?

Correct, where the .js would be the autoloader.


Seems for me, the cleanest option would be if ncc at a minimum supported the module field. I think that the module field is desirable for ncc, as to my knowledge, ESM provides better tree shaking abilities than CJS.

Would be even better if ncc supported the editions format, that way they could include the typescript source if available, rather than compiled code. Just by trickling through the editions until they find the one they support.

@styfle
Copy link
Member

styfle commented Apr 9, 2019

Seems for me, the cleanest option would be if ncc supported the module field

The module field in package.json is not used by Node.js so I don't think it's safe to rely on this.

There will be a new type in package.json field but it's still experimental as of nodejs/node#26745

@balupton
Copy link

balupton commented Apr 9, 2019

For an .mjs main are you thinking of something like "main": "x" where the .mjs or .js version is selectively used depending on the environment support? It isn't yet clear whether this pattern will ever be supported for Node.js,

Seems .mjs is still supported with the proposal mentioned by @styfle

nodejs/node#26745 (comment)

As such, .mjs is how one would maintain compatibility with old node versions. With packages that don't care about b/c using type: module and pointing main to an ESM, breaking compat with old node versions.


The advantage of module is that it already has wide bundler support, https://www.pikapkg.com being the leading example.

@guybedford
Copy link
Contributor

Seems .mjs is still supported with the proposal mentioned by @styfle

.mjs is supported, yes, but only when explicitly used in the main field - "main": "x.mjs".

The ability for the default extension for a "main": "x" to act as a dual-mode split between CommonJS and ES modules is still under debate in the modules working group.

@balupton
Copy link

balupton commented Apr 9, 2019

The ability for the default extension for a "main": "x" to act as a dual-mode split between CommonJS and ES modules is still under debate in the modules working group.

So their goal does not include backwards compatibility?


Regardless of the working group. I think it is important that packages that work today also work with bundlers. So far the only solution provided is an eval trick, that increases burden on package authors and increases complexity with other bundlers.

If ncc added the module field, like many bundlers now do, or added mjs support, then life would be easier, without concern for the working group's complications.

At this point, my builds are failing and any package that uses editions will fail with zeit's now and ncc.

This leaves the options as:

  1. I need to update hundreds of packages to use mjs, or module field, or eval complexity, and update boundation to make such updating of hundreds of packages easier - eval being the highest cost option
  2. Or, ncc supports editions field, and editions users don't have to do anything, and the ecosystem can progress to a better place
  3. I and those consuming bevry's packages leave the zeit ecosystem

Other bundlers don't have this issue, as they use the browser field, which the hundreds of editioned packages already provide.

@rauchg
Copy link
Member

rauchg commented Apr 9, 2019

I and those consuming bevry's packages leave the zeit ecosystem

And the node.js ecosystem without custom bundlers, correct?

@tunnckoCore
Copy link
Author

tunnckoCore commented Apr 10, 2019

Just to throw little comment here again :)
I don't get what can be considered "not safe", because it would be an optional option and not a default. Those who choose to use it will know what they do and why they do it. The module field convention is huge enough and used for years, and don't see a problem to support it until "the standard" which may or may not come with the new type field. Also it's not guaranteed if and when it can become reality (not followed the thread there yet).

When I did a fork with this small change and used it for couple of packages it worked perfectly, even successfully bundled eslint and such.

@guybedford
Copy link
Contributor

I guess we could support a "module": true option, although I really wouldn't recommend it, especially with pika using that now to mean "browser + module", where ncc would want "node + module".

But I don't think @bevry is asking for the "module" field either though.

@balupton
Copy link

balupton commented Apr 10, 2019

I guess we could support a "module": true option, although I really wouldn't recommend it, especially with pika using that now to mean "browser + module", where ncc would want "node + module".

Good point. Really makes me think that https://editions.bevry.me approach of each edition having tags that can be used for language and module system, as well as an engines field, is the right away to go about this.

How hard would it be to add editions support to ncc? Could it be done as a plugin install and a one line configuration option? If pointed in the right direction, I could find time to do it myself, or place a bounty/commission for its development.

Also thank you for the dialogue. Keen to get a working approach happening.

@styfle styfle changed the title Support module-first resolving, instead of forcing main-first Support for ECMAScript Modules (ESM) Apr 10, 2019
@styfle styfle added blocked enhancement New feature or request labels Apr 10, 2019
@tunnckoCore
Copy link
Author

Any progress on this?

It really worths nothing to set mainFields: opts.mainFields || ['main', 'module'].
The current behaviour will still be the default. If you so much want it, you can put it in the README in bold "it's not recommended" for whatever reason.

@tunnckoCore
Copy link
Author

tunnckoCore commented Jun 10, 2019

That's it. tunnckoCore@2d83d3a (feat/add-main-fields so you can merge it when/if you want).

I'll maintain the fork for now.

And one another strange thing: why there is no ESLint or Prettier setup here? Additionally in my fork added prettier.

@styfle
Copy link
Member

styfle commented Sep 9, 2021

Fixed in #720 by detecting .mjs or type: module

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants