BUG MITIGATION: .mjs should not resolve before .js files

[leebyron]

Support for .mjs files added in #3239 did not account for npm libraries which ship native mjs files alongside js files. This accounts for this by ensuring .js files resolve before their accompanying .mjs file. Note that this is not an ideal end state since selecting a .mjs over a .js extension should be the result of whether import was used instead of require() in a node environment with native ESM support (currently via --experimental-modules). Instead, this change just always selects a .js extension before the .mjs extension if it exists.

This unbreaks support for using GraphQL (relay, apollo, etc) within create-react-app projects.

[leebyron]

cc @gaearon @Timer - it would be great if we could get this into a point release soon

[Timer]

Hmm, is this a new problem (as in did these modules just now start shipping mjs and did not previously)?
This is the first we're hearing of this bug report.

[leebyron]

Yes, some graphql packages just started releasing node-esm ready modules to npm for support with node v9's --experimental-modules and in anticipation of node v10.

Node has a new module resolution strategy which will resolve to a .mjs file from a native import statement, but continue to resolve to a .js file from a require(). To support both, npm modules can be published that have both .js and .mjs variations of each file in the bundle.

However this conflicts with react-scripts's jest-config which before this diff always resolves to .mjs if one exists before .js, even if the environment is not using native modules - which results in seeing syntax errors like reported in graphql/graphql-js#1248

Since Jest wraps the require() flow, and not the import flow, I believe Jest doesn't have support for native node-esm just yet, however resolving .mjs files when a .js file isn't found is still a reasonable thing to do since user's source code may start using .mjs along with the babel transform (as opposed to node_modules code which is not transformed by babel)

[iansu]

This seems like an easy fix. We should get this into master as well as next.

[Timer]

From my understanding, this problem most likely arises because imports are happening without specifying file extension.

When Jest loads a package, we look at the main key in package.json, when webpack loads a package, we look at the module key and fallback to main.

In a normal scenario, main would contain index.js and module would contain index.mjs.

These files would respectively import etc.js and etc.mjs files, but instead, specify just etc, which falls back to our module resolve ordering which disregards the means of import.

Given this reasoning, I'd go as far to say that this isn't the fix we want -- we can release this in a 1.x.y release for compatibility reasons, but I'm against porting this to next because it's not the desired behavior.

I'd like to know how Node handles this, given an import specifying just etc, does it prefer etc.js when invoked via require and etc.mjs when invoked via import? What's the fallback look like? What happens when using import()? Does Node itself bomb out in this situation?

We definitely need to figure this out for the 2.0 release.

@leebyron can you retarget this PR to the master branch please? We'll want a different solution for next as described above.

[leebyron]

Give me a moment to change the base - I can't do that from the github UI without wreaking havoc.

Before I do that, let me try to answer some of your questions.

I'd like to know how Node handles this

Node handles this by never attempting to load a .mjs file from a require() and never loading a .js from an import. So import "./example" may try to look for: ./example, then ./example.mjs, or ./example/index.mjs, but never ./example.js. Similarly require("./example") may look for ./example.js but would never look for ./example.mjs. In other words, the existing module resolution algorithm for require() was not changed at all, however a new module resolution algorithm was introduced for import.

Given this reasoning, I'd go as far to say that this isn't the fix we want

I definitely agree - this is a mitigation. I would argue that .mjs files should not be considered at all from a require statement in Jest - instead Jest should hook Node's new import resolution API in which case it should look for .mjs files. Though I know that we're still in the strange world of babel transpilation and so this isn't always very well defined, and that folks may choose to save their ES6+ESM files as .mjs files.

This mitigation is specifically handling the case where both ./example.js and ./example.mjs exist in the same location. The .mjs file is going to include ESM syntax, and if it happens to be found in the node_modules folder then babel isn't going to transform it. In this scenario instead of resolving the .mjs file first, we should be resolving the .js file first.

Tools like Webpack and Rollup should be doing the opposite - looking for .mjs files before .js files - since they prefer the ESM format if it exists.

When Jest loads a package, we look at the main key in package.json, when webpack loads a package, we look at the module key and fallback to main.

Yes, and node also only looks at the main key in package.json, regardless of whether require() or import was used to arrive at that package.json file - which is why this pattern of .js and .mjs files next to each other is likely to become more common as native ESM moves from experimental to shipping.

[Timer]

I'm trying to figure whose court the ball is in to handle this (for 2.0).

I could see arguments for us doing something complex, I could also see arguments for Jest to handle this.

It's almost as if we need a moduleFileExtensions and esModuleFileExtensions key to specify extensions for require/import.

[leebyron]

I like the idea of a separate esModuleFileExtensions. I think this should probably be within Jest's domain. If it encounters an npm module using native ESM then Jest should be defining the right behavior. There's a bit of runway to get this right in the meantime. Node v9 only supports native ESM with --experimental-modules, so it's not going to be all that common until the major version that supports it without a flag.

[leebyron]

Tools like Webpack and Rollup should be doing the opposite - looking for .mjs files before .js files - since they prefer the ESM format if it exists.

Just checked this out and Webpack does look for .mjs before .js

[jbaxleyiii]

This also killed mjs efforts for the Apollo ecosystem 😢

[Timer]

We don't want to rush these changes out since there's not a ton of +1s.

Let me reiterate -- this is completely solvable in user land in the mean time (which shouldn't be long).

Source files just need to be updated to explicitly import their file.js or file.mjs counterpart instead of file.

[jbaxleyiii]

@Timer so you are saying that if foo.mjs changes import bar from ./bar to import bar from ./bar.mjs it would work?

Is that in the lib or in user code?

[leebyron]

To be clear this cannot be mitigated in user land. This blocks using the most recent version of GraphQL.js along with create react app.

The critical issue is loading files from node_modules, where it is not clear how to rewrite import x from “graphql” to explicitly use JS files.

This problem is made worse within cross dependencies. Projects like Apollo and Relay shouldn’t be forced to change their imports to require(“graphql/index.js”)

[leebyron]

I definitely support coming up with the right long term plan, which probably incorporates adding support for native ESM to jest, however in the meantime I still would love to see this merged and released as soon as possible since it’s a low impact bug fix that unblocks using CRA with the graphql ecosystem

[iansu]

This definitely sounds bad. Is this only broken when running tests via Create React App? Is it only the Jest config that needs to be updated or is there also an issue with our webpack config?

[stevensacks]

@leebyron Please release this.

[gaearon]

To clarify, Lee just reported and sent a fix for this issue, we're the ones who should release it

[gaearon]

Out in 1.1.2.

[gaearon]

(Sorry it took me a long time)

[nfantone]

So, in the end, this didn't target next, but master, if I'm not mistaken? Latest 2.0.0-next does not have this fix?

[iansu]

That's correct. This fix is really just a hack. We're hoping that Jest and webpack will properly support .mjs in the near future and that we can incorporate those new versions in 2.0.

[jefflau]

Is there a way of getting this fix into next? I'd like to use this along with babel-plugin-macros by @kentcdodds

[Timer]

This fix is really just a hack.

We have no plans of merging this into 2.0 unless it's the last remaining blocker for a 2.0-final release.

[leebyron]

I think it would be a mistake to regress on this bug in CRA v2

That's correct. This fix is really just a hack. We're hoping that Jest and webpack will properly support .mjs in the near future and that we can incorporate those new versions in 2.0.

I don't know that I would agree with this characterization. This PR was a bug fix against CRA for Jest and webpack's semi-hacky implementation of ESM. Jest and webpack will probably require extensive architectural changes requiring major version bumps to properly support native ESM. I wouldn't suggest holding your breath for that. If CRA v2 intends to ship with support for Jest and Webpack's current major version then it should include this bug fix.

[gaearon]

I agree with @leebyron here

[stellarhoof]

I see a fix has been merged for this issue in 0e0f260, but the latest release of next I in npm is behind, at 66cc7a9. Will it be updated any time soon?