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

mjs files get imported as static files #5234

Closed
jgoux opened this issue Oct 2, 2018 · 15 comments · Fixed by #5258
Closed

mjs files get imported as static files #5234

jgoux opened this issue Oct 2, 2018 · 15 comments · Fixed by #5258

Comments

@jgoux
Copy link

jgoux commented Oct 2, 2018

Hello,

Here is a repro :

import graphql, { parse, buildASTSchema, buildClientSchema } from 'graphql'
console.log(graphql, parse, buildASTSchema, buildClientSchema)
// ouput : /static/media/index.497f5ee2.mjs undefined undefined undefined

Excluding the .mjs extension from the file-loader fix the issue.

@gaearon
Copy link
Contributor

gaearon commented Oct 3, 2018

I am so lost. 😥

Can somebody who has a stake in getting mjs working with Create React App (e.g. Apollo, graphql package, etc) please comment here and suggest the proposed plan for how we should be interpreting it?

cc @lencioni @jgoux @hobochild @leebyron @jdalton @jbaxleyiii @jaydenseric who participated in earlier threads

So far mjs has created nothing but churn for us and our users, and I'm still not sure what are the intended benefits of this shift at this point time. I empathize with the desire to be ESM-compatible when Node removes the flag, but I'm also confused about what we should be doing right now in Create React App.

Our constraints are:

  • we need to support both Jest and webpack
  • Jest probably won't have native ESM support for a while
  • we can't afford to completely break when a library releases an mjs version side by side
  • (?) I guess it's reasonable that libraries shipping only mjs won't work in Jest?

We've intentionally tried to disable mjs support, as discussed in #4085 (comment). But apparently this is still not enough because graphql specifies mjs as a module field thus still bringing it in even though we've decided against supporting it. So we have no other option than to support it, then?

@jaydenseric
Copy link

jaydenseric commented Oct 3, 2018

because graphql specifies mjs as a module field thus still bringing it in even though we've decided against supporting it. So we have no other option than to support it, then?

Yes, at least when bundling dependencies in node_modules. You can decide however not to support .mjs in project files, that is the level of support Next.js has.

Out of the box Webpack handles .mjs in the module field just fine, you run into problems when you customise the resolve.extensions field and explicitly leave out (or misorder) .mjs.

See jaydenseric/apollo-upload-client#118 (comment)

@gaearon
Copy link
Contributor

gaearon commented Oct 3, 2018

Does #5258 look like the right fix for you?

@gaearon
Copy link
Contributor

gaearon commented Oct 3, 2018

Is there a set of packages that we can test our support on? To verify we don't keep breaking this in every release.

@Timer
Copy link
Contributor

Timer commented Oct 3, 2018

#5258 should get us back on track to supporting what will be "properly" setup packages in a Node & web world.
graphql is probably the best package to test with: it supports CommonJS and ESM, properly toggling between ESM or CommonJS based on what the Node environment supports.

Can someone with more knowledge in this area please confirm this is true?

@jaydenseric
Copy link

The CRA config is a little more complex than I'm used to… does CRA attempt to transpile all of node_modules? If so, does it leave out the CJS/ESM transforms for Webpack to handle? Babel transpiling the ESM in an .mjs file might mess with Webpack's ability to do proper CJS interop when bundling.

@Timer
Copy link
Contributor

Timer commented Oct 3, 2018

CRA compiles all of node_modules, yes. We leave out the ESM => CJS transform so webpack handles it.

@jdalton
Copy link

jdalton commented Oct 3, 2018

@Timer

graphql is probably the best package to test with: it supports CommonJS and ESM, properly toggling between ESM or CommonJS based on what the Node environment supports.

It's still pretty hard to get things right with the current state of things. For example, even graphql has issues working with --experimental-modules with mixed CJS and ESM use. It'd be helpful to avoid any solution that makes .mjs viral (requiring consumers, or deps of deps to be renamed to .mjs to get things bundled properly).

Projects like Babel 7, webpack 4, and esm restrict .mjs use or implement more-strict rules when handling experimental .mjs files. However, Babel and webpack can be configured to handle .mjs in less-strict ways. I think complications may come from webpack enforcing its javascript/esm for .mjs by default, instead of javascript/auto for the module type.

@jaydenseric
Copy link

jaydenseric commented Oct 3, 2018

.mjs intentionally sends a very strong signal that the mode is to be esm, and for how CJS may be imported. No one should be putting anything other than correct ESM in an .mjs file, and it is a grave mistake to facilitate or encourage misuse.

It is far preferable that a bundling error throws so people can fix (and learn about) their flawed .mjs or stop using it, rather than mask issues and then the .mjs won't run natively in Node.js or in other tools that do fairly assume that .mjs means ESM.

@jdalton The graphql issue you are talking about is not relevant to this issue.

@jdalton
Copy link

jdalton commented Oct 3, 2018

@jaydenseric

The graphql issue you are talking about is not relevant to this issue.

My comment was highlighting that support is tough even for projects which appear to be doing things properly.

.mjs intentionally sends a very strong signal that the mode is to be esm, and for how CJS may be imported. [...] rather than mask issues and then the .mjs won't run natively in Node.js

I agree with caution over .mjs use. It's experimental in Node and what it will-and-will-not support isn't fully realized yet. Unfortunately I believe early adopters have jumped to .mjs a bit prematurely without considering the consequences and complications of such move. The ecosystem is in tough place trying to support the mix of strict and loose flavors of ESM. Ultimately, because of early adoption, for things to keep working I think the ecosystem will end up treating .mjs use of today more like the looser babelish-.js.

@jaydenseric
Copy link

for things to keep working I think the ecosystem will end up treating .mjs use of today more like the looser babelish-.js.

I haven't seen that happening, it defies the point of .mjs; which is so ESM can run natively in Node.js v8.5+ in --experimental-modules mode. When it is done incorrectly, it is by accident as it won't work and the sooner tools pickup accidental errors the better.

Node.js v10 end-of-life is April 2021, so like it or not .mjs will be in use for at least 2.5 years more, possibly forever if it happens to pan out as the only practical solution for ESM in Node.js. I've been following the nodejs/modules working group and despite months of circular debates there have been no concrete decisions to action anything differently. Frankly I'm waiting for Deno to mature to abandon the whole Node.js/npm CJS ecosystem altogether and write much more isomorphic code.

Discourage use of an "experimental" feature, sure. But I can't get my head around why anyone would encourage incorrect use of .mjs; there is no benefit to the ecosystem and it makes things more complex, not simpler.

.mjs support is not as difficult as is made out; I've achieved it for 4+ of my own published packages. The more people doing it the easier it gets. The deliberately stunted adoption is what is making this difficult, if --experimental-modules was unflagged and .mjs officially encouraged all the issues would be worked out in the first year of mass adoption and within a few more years CJS would be history and issues like graphql/graphql-js#1479 would be irrelevant.

Anyway, speculating about the service life of .mjs doesn't help to resolve this perfectly solvable CRA issue: "mjs files get imported as static files".

@Timer
Copy link
Contributor

Timer commented Oct 3, 2018

@jaydenseric / @jdalton can you confirm my conclusion is somewhat right? #5258 (comment)

@Timer Timer reopened this Oct 3, 2018
@Timer Timer modified the milestones: 2.0.4, 2.0.x Oct 3, 2018
@Timer
Copy link
Contributor

Timer commented Oct 3, 2018

Re-opened for continued discussion.

@jdalton
Copy link

jdalton commented Oct 3, 2018

@jaydenseric

which is so ESM can run natively in Node.js v8.5+ in --experimental-modules mode. [...] Node.js v10 end-of-life is April 2021 [...] The deliberately stunted adoption is what is making this difficult, if --experimental-modules was unflagged

👋 Node core and Node Module WG member here. Just a heads up, there is no set date for when --experimental-modules will be unflagged. There's also no guarantee that what has shipped now is what will be in the future. The Node Module WG does not recommend using --experimental-modules for production use.

Discourage use of an "experimental" feature, sure. But I can't get my head around why anyone would encourage incorrect use of .mjs; there is no benefit to the ecosystem and it makes things more complex, not simpler.

Bundling mushes transpiled code, CJS, ESM, and .mjs code together. Things can get a little funky.

Anyway, speculating about the service life of .mjs doesn't help to resolve this perfectly solvable CRA issue: "mjs files get imported as static files".

👍

@Timer

can you confirm my conclusion is somewhat right?

Yep. It'll do 😎

@Timer
Copy link
Contributor

Timer commented Oct 4, 2018

This should be resolved. It seems everyone has spoke their peace. Thanks!

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