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

esm loads mjs file extensions by default #575

Closed
dnalborczyk opened this issue Aug 20, 2018 · 8 comments
Closed

esm loads mjs file extensions by default #575

dnalborczyk opened this issue Aug 20, 2018 · 8 comments
Labels

Comments

@dnalborczyk
Copy link
Contributor

dnalborczyk commented Aug 20, 2018

I'm running npm ci && find . -name '*.mjs' -delete for quite a long time, and forgot to file an issue. esm is loading mjs file extensions by default. I assume this is not intended?

this is not a problem per se, the problem arises when other 3rd party modules depending on 'graphql' make e.g. a type or reference check.

hypothetically, I don't recall the exact issue:

  • graphql[.js+mjs] exports a type, e.g. GraphQLScalarType
  • graphql-tools, which depends on graphql, but loads the cjs|js version, and some functionality checks if the constructor is of type: GraphQLScalarType
  • consumer app, depends on graphql and graphql-tools, is using the GraphQLScalarType (from, as it stands, the mjs package), and graphql-tools fails to check for the constructor (because it got it from the cjs package)

repro:

npm i graphql
// import anything
import { GraphQLScalarType } from 'graphql'

although the graphql package has a module field "module": "index.mjs" if one would remove it it's still loading the mjs files.

@jdalton on a different (related) note: is esm always looking into package.json module fields, or only if explicitly specified in the mainFields option?

@jdalton
Copy link
Member

jdalton commented Aug 20, 2018

Hi @dnalborczyk!

In this case what you're seeing isn't related to mainFields it just looks that way. By Node ESM rules it prioritizes loading .mjs over .js when the file extension for a file is not specified.

In your example graphgl has a main field of "main": "index". Since index doesn't have its extension specified by Node ESM rules it will look for index.mjs before index.js which is why it loads index.mjs with esm.

The type and reference check issues should be filed on graphql as it's one of the very real ecosystem issues having 2 execution points (one for cjs as index.js and one as esm for index.mjs) can create.

on a different (related) note: is esm always looking into package.json module fields, or only if explicitly specified in the mainFields option?

By default mainFields is ["main"].

@jdalton jdalton closed this as completed Aug 20, 2018
@dnalborczyk
Copy link
Contributor Author

ah, ok! thanks a bunch for the rundown!

I'll get back to the graphql package. essentially, what they would need to do is specify "main": "index.js". would that be sufficient enough? I'm sure graphql with other 3rd party modules will have similar execution point problems with --experimental-modules.

@jdalton
Copy link
Member

jdalton commented Aug 20, 2018

I'm sure graphql with other 3rd party modules will have similar execution point problems with --experimental-modules.

Yes. Their use of index (with no extension) is by design. They are trying to do a dual package. One that loads .js in Node that doesn't support ESM and one that loads .mjs in Node that does (using --experimental-modules for now).

I'll get back to the graphql package. essentially, what they would need to do is specify "main": "index.js". would that be sufficient enough?

Since it's by design you likely need to address the root issue. This is where dual packages fall apart. Since they have 2 entry points that execute 2 different code paths they get 2 different instances of their modules (makes those type and reference checks fail). In order for them to solve this they'd need to write the shared parts in CJS so it's a single instance.

@dnalborczyk
Copy link
Contributor Author

I think they should remove the .mjs files, or rely on the "modules" field for the time being, since that 'mjs' stuff is also still in flux.

Since it's by design you likely need to address the root issue.

just hit the root issue again. I'll come up with some use case for them.

In order for them to solve this they'd need to write the shared parts in CJS so it's a single instance.

that makes me think where esm would come in handy. creating a bridge to es modules, it would also still have the same references in both (bridge & es) cases (I'd assume). it would be awesome if packages would adopt esm and bridged es modules, which I believe was one of the goals for esm all along.

speaking of which, are you still planning on releasing lodash v5 with esm? that would set a great precedence.

Though I think there's still some things to be ironed out regarding mainFields and things like that. I have to file a couple questions/issues/bugs on that topic.

@jdalton
Copy link
Member

jdalton commented Aug 20, 2018

I think they should remove the .mjs files, or rely on the "modules" field for the time being, since that 'mjs' stuff is also still in flux.

Yes. You can point to a non-me Module WG comment too.

just hit the root issue again. I'll come up with some use case for them.

I think someone may have already opened an issue on it for them. So you can pile on that one.

that makes me think where esm would come in handy.

Since issues like this will become more of a thing. I might want to add a options.cjs to resolve .js first so that I can bypass the dual instances with esm and folks can use these packages. I'll have to think if that will give them a false sense of OK-ness with .mjs though. I don't want to hide the real pain point of .mjs there.

speaking of which, are you still planning on releasing lodash v5 with esm? that would set a great precedence.

Yep. It's the only way Lodash v5 will ship.

Though I think there's still some things to be ironed out regarding mainFields and things like that. I have to file a couple questions/issues/bugs on that topic.

Ya probably. At the moment if a non-"main" options.mainFields resolves to .mjs I ignore it (again not wanting to make .mjs more usable).

@dnalborczyk
Copy link
Contributor Author

cool, thanks a lot!

@jdalton
Copy link
Member

jdalton commented Aug 28, 2018

Pinging back to link to an opened graphql issue related to their dual entry and instance issue: graphql/graphql-js#1479

@dnalborczyk
Copy link
Contributor Author

ah, nice, thanks @jdalton !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants