Skip to content
This repository has been archived by the owner on Aug 4, 2021. It is now read-only.

Support .mjs files by default. #151

Merged
merged 1 commit into from May 16, 2018
Merged

Conversation

leebyron
Copy link
Contributor

Since rollup prefers ESM formatted modules, it should look for node's new .mjs file extension before looking for .js files by default. This offers support for deployed modules intended to be used in node's ESM mode.

Folks can work around this today by manually supplying the extensions option, however it would be great if this worked by default.

Note that looking for .mjs before .js is important for rollup which prefers ESM, however the resolve dependency should not use the same order by default since it is used in many other tools which do not yet support ESM or would not expect it by default.

Encountered this as the root cause behind graphql/graphql-js#1293

Since rollup prefers ESM formatted modules, it should look for node's new .mjs file extension before looking for .js files by default. This offers support for deployed modules intended to be used in node's ESM mode.

Folks can work around this today by manually supplying the `extensions` option, however it would be great if this worked by default.

Note that looking for `.mjs` before `.js` is important for rollup which prefers ESM, however the `resolve` dependency should not use the same order by default since it is used in many other tools which do not yet support ESM or would not expect it by default.

Encountered this as the root cause behind graphql/graphql-js#1293
@leebyron
Copy link
Contributor Author

Ping on this - this unfortunately still blocks use of Rollup for anyone using GraphQL or any other native ESM module.

@TrySound
Copy link
Member

Why graphql just didn't use existing module field? Now everybody shows graphql as an example. mjs sucks.

@leebyron
Copy link
Contributor Author

@TrySound, while packagers like Rollup and Webpack use the module field, it’s not how node’s native module loader designed.

Part of the issues is that it is not just the entry point into a package that can be differentiated but also any other file or subpath into a package. I know the team working on a native module loader considered all of these issues when arriving to the .mjs behavior.

This PR doesn’t affect the existing support for a package’s top level module field, but adds support for packages built in a “dual mode” way.

This is a good article capturing all of that in a much deeper way than I’ve done here and further explaining “dual mode” https://medium.com/@giltayar/native-es-modules-in-nodejs-status-and-future-directions-part-i-ee5ea3001f71

@leebyron
Copy link
Contributor Author

leebyron commented May 16, 2018

Cc @keithamus @Rich-Harris any chance you could include this in a next release?

@TrySound TrySound merged commit 74f2e41 into rollup:master May 16, 2018
@TrySound
Copy link
Member

Thank you.

@ansarizafar
Copy link

Is this feature released? I am still not able to use graphql/ApolloServer with rollup.

@keithamus
Copy link
Contributor

keithamus commented Sep 4, 2018

Just shipped 3.4.0 which includes this fix.

@aMarCruz
Copy link

aMarCruz commented Dec 26, 2018

he, in some package.json props of my latest libraries...

  • main: index.js => CJS+ES5 for brunch, TypeScript, and some browserify configs.
  • es2015: index.mjs => ESM+ES7 for node 10+ by extension, latest browsers by <script type="module">
  • module: index.es.js => ESM+ES5 for bundlers (rollup, etc)
  • browser: index.min.js => UMD+ES5

NodeJS import of .mjs is async and very different of Rollup's import, but it is really a nightmare. I just need to document this behavior.

@TrySound
Copy link
Member

TrySound commented Dec 26, 2018

@aMarCruz I don't recommend to keep UMD in browser field. Browser field is preferred by all bundlers over main and module fields. You may use a field like unpkg for it.

es2015 field for es7? I'd say the idea of that field is not good from the beginning.

A lot of uber packages use esnext field. By the way they use browser field to distinct node and browser versions of the script.
https://github.com/uber/react-map-gl/blob/master/package.json#L19

Also use please esm extension for module field. It's more descriptive about format. A lot of tools like esm and rollup itself (format alias) already adopted it.

@aMarCruz
Copy link

aMarCruz commented Dec 26, 2018

@TrySound thanks, I'm already documenting the unpkg link in the Readme, but you may right and I'm thinking about to remove the "browser" field and adopt your recomendations for the rest.

The es2015 is used by Angular for ESM+ES7

@TrySound
Copy link
Member

Well do you agree this is weird? es2015 is es6, es2016 is es7. Sticking with specific version is confusing.

@aMarCruz
Copy link

aMarCruz commented Dec 26, 2018

my bad, it is for ESM+ES6, but I agree, it is something strange comming from Google.

added: this are the formarts that I'm using: https://github.com/ProJSLib/jsbits#distribution-formats

@gaberudy
Copy link

For those following the breadcumbs of why graphql can't be used with rollup, although the index.mjs file is loaded from graphql, it re-exports from a bunch of sub-modules, each of which has both a index.js and a index.mjs file.

rollup seems to always check for index.js first and use it if it exists.

My solution, pre-running rollup, I do:

rm   node_modules/graphql/*/index.js

which allows rollup to pick up the index.mjs files and then it can find all of it's imports

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants