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

Add .mjs support back to webpack (it has native ESM support) #5258

Merged
merged 4 commits into from
Oct 3, 2018

Conversation

Timer
Copy link
Contributor

@Timer Timer commented Oct 3, 2018

Fixes #5234.
Closes #5257.
Closes #5235.

Webpack was never the problem with .mjs support, so we probably shouldn't of removed it.

We still want to figure out a story for Jest, we're just not sure what that is yet.


I opted to resolve .mjs before .web.js this time, does this seem OK?

@Timer Timer added this to the 2.0.4 milestone Oct 3, 2018
@Timer Timer changed the title Add .mjs support back to webpack, because webpack was not the problem Add .mjs support back to webpack (it has native ESM support) Oct 3, 2018
@Timer Timer requested a review from gaearon October 3, 2018 00:20
Copy link
Contributor

@gaearon gaearon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make sure we test this with packages that created problems before? And that they work in Jest due to the fallback?

@Timer
Copy link
Contributor Author

Timer commented Oct 3, 2018

We can use the existing behavior tests and add a few more, making sure GraphQL & friends work.

@Timer
Copy link
Contributor Author

Timer commented Oct 3, 2018

Here's my understanding:

The ecosystem is currently embracing experimental mjs support due to widespread adoption of ESM, fueled by the early days of Babel.
Turning off this support holistically is not an option (like we tried to do in v2). It's just not practical, and still begs questions about how ESM in .js files should be handled.

.mjs is the specification and nitty-gritty details on how ESM will work in Node, meaning we should align as closely to this behavior for .mjs files.
This specification currently mandates that ESM can only import the default export from CommonJS.

This constraint has been applied to the following systems:

  1. TypeScript 2.7 added a new mode close in line to this proposal.
  2. webpack 4 added more spec-compliant handling of ESM.
  3. Babel added a spec mode, in line with above behavior, only permitting importing the default export from CommonJS.

webpack 4's default behavior is to enforce spec for files with the .mjs extension, only allowing the default import to be imported from CommonJS.
webpack 4 still allows .js files containing ESM to import "named exports" from CommonJS.

This behavior aligns closely with what we currently see in the ecosystem:

  1. Legacy ESM interop/previously compiled ESM modules are published under .js extensions
  2. Node-specified interop between ESM and CJS will lives under .mjs extensions, which has strict behavior

I believe setting javascript/auto will just make the .mjs adoption story worse, either forcing Node to support bad semantics or dramatically increasing the churn when .mjs is finalized.

Given all the information provided above, I suggest we take the following path:

  1. We add support for .mjs files back to webpack, and resolve them before all other extensions
  2. We leave .mjs support on in strict mode -- this will ensure proper compatibility when Node finalizes the specification and we have to start wondering about how usage with Jest will look
  3. We attempt to add no support for mjs to Jest or our Jest configuration until the Node specification is no longer experimental and ships in a LTS version of Node
    • This will encourage users to continue shipping CommonJS versions of their package while the specification settles. If the fickle finger of fate ruins everything, we can always revert back to CommonJS. 😄
  4. We discourage users from using the .mjs extension in their project code, as it will be problematic to test

Potential issues:

  1. The browser field is recognized before module, meaning if it points to index.js and index.js imports ./foo, webpack will import ./foo.mjs before ./foo.js.
    • This behavior could be problematic if the published package differentiates behavior between mjs and js files, but this is not likely
  2. Users start shipping packages without CommonJS available under the main key in package.json as a result of this behavior, further reducing the ability to test with Jest (we're already seeing this, probably not a concern)
  3. I'm OK with this causing some churn for a few packages which may have opted into using mjs but didn't vet their dependencies having properly exported/available .mjs distributions. These dependencies might have to replace named imports with default imports.

@jaydenseric
Copy link

The browser field is recognized before module, meaning if it points to index.js and index.js imports ./foo, webpack will import ./foo.mjs before ./foo.js.

I'm not following the issue… .mjs is suitable for bundling via Webpack for use in browsers too. Can't a dependency of a dependency also provide a browser entry, if necessary?

@jaydenseric
Copy link

Do you mean the browser entry will start importing from the server files by accident? That shouldn't happen with this structure:

lib/
├── server/
│   ├── index.mjs
│   └── index.js
└── browser/
    ├── index.mjs
    └── index.js
{
  "browser": "lib/browser",
  "main": "lib/server"
}

@Timer
Copy link
Contributor Author

Timer commented Oct 3, 2018

The browser entry may not be pre-bundled into a separate tree or dist file.
If it's a .js file that requires other files that don't specify an extension, it'll be automatically promoted to .mjs.

I'm imagining a package.json like this:

{
  "main": "index",
  "module": "index.mjs",
  "browser": "index.js"
}

And a result of this:

Just a thought.

edit: we posted at the same time

Yes, that structure fixes it. But graphql is a perfect example of these files being shipped side by side.

https://unpkg.com/graphql@14.0.2/language/


I mentioned (maybe not clearly) that I don't think this would happen often, so it's probably nothing we need to be worried about. It's just a possible (not probable) concern.

@jaydenseric
Copy link

Not following what you mean with graphql, they are meant to be sibling .mjs and .js files. It does not have a browser field, so both .mjs and .js files should be ok to bundle for use in a browser. .mjs is preferable, so Webpack can treeshake.

@Timer
Copy link
Contributor Author

Timer commented Oct 3, 2018

I used graphql just as an example, pretend it had a browser field and pretend that behavior was different because .mjs was intended for Node and .js was more general purpose.
This is a completely fabricated hypothetical, and one I don't think we need to be concerned about.

It's just a possible (not probable) concern.

The reason I say this is a concern is because (AFAIK) Node will resolve .mjs before .js or .js before .mjs depending on the calling file type -- this behavior isn't possible in webpack, you get one resolution order.

@jaydenseric
Copy link

Just a side note, but I think with an extensionless main entry and sibling .mjs/.js files, the module entry is kind of redundant, since webpack would have got to the same .mjs file following either entrypoint. I might remove the module field from my packages at some point.

@jaydenseric
Copy link

jaydenseric commented Oct 3, 2018

Also, this example was incorrect:

{
  "main": "index.js",
  "module": "index.mjs",
  "browser": "index.js"
}

Node.js only looks at the main field, so it needs to be extensionless:

{
  "main": "index",
  "module": "index.mjs"
}

^ in that example you might have even gotten away with no main or module entry at all; Node.js and Webpack look for index anyway I think.

@Timer
Copy link
Contributor Author

Timer commented Oct 3, 2018

I'm definitely a proponent of dropping module once main is extensionless with side-by-side files.

@Timer
Copy link
Contributor Author

Timer commented Oct 3, 2018

I'm going to land this because it aligns with my latest understanding of this.
If we uncover more information or this breaks everything, I'm more than happy to make .mjs use javascript/auto and instead, make this a breaking change in the next version of CRA.

For now, we'll classify this as a bug fix. Cases where this would be considered breaking probably aren't working right now anyway.

We still need to come up with a good testing story for this (e2e).

@Timer Timer merged commit 736561f into facebook:master Oct 3, 2018
@Timer Timer deleted the add-back-mjs branch October 3, 2018 04:03
@Timer
Copy link
Contributor Author

Timer commented Oct 3, 2018

Tested via #5263.

@jdalton
Copy link

jdalton commented Oct 3, 2018

@Timer

TypeScript 2.7 added a new mode close in line to this proposal.

That mode aligns TS more with traditional Babel in terms of the __esModule property, enabling support for named exports of CJS modules, and more correct handling of namespace objects. So not related to .mjs or --experimental-modules rules.

Given all the information provided above, I suggest we take the following path:

  1. We add support for .mjs files back to webpack, and resolve them before all other extensions
  2. We leave .mjs support on in strict mode -- this will ensure proper compatibility when Node finalizes the specification and we have to start wondering about how usage with Jest will look

It would be wonderful if there was a way to give a heads up to folks that using .mjs, in the context of Node, is experimental and should not be used in production (including bundles). I suspect some may have read to use .mjs for ESM code and may not be aware of its complications in Node or tooling.

  1. We attempt to add no support for mjs to Jest or our Jest configuration until the Node specification is no longer experimental and ships in a LTS version of Node
    • This will encourage users to continue shipping CommonJS versions of their package while the specification settles. If the fickle finger of fate ruins everything, we can always revert back to CommonJS. 😄

👏

  1. We discourage users from using the .mjs extension in their project code, as it will be problematic to test

I've done something similar, but less explicit, with the esm loader by limiting functionality. There is no way to enable CJS interop (module vars, mocking, stubing, spys, code coverage, APM, etc.) or experimental features (like top-level await) with .mjs. More explicit opt-ins, like Node's experimental flag, may be needed for tooling like esm, Babel, and webpack to help inform developers when they're stepping into experimental territory for their packages and applications.

zmitry pushed a commit to zmitry/create-react-app that referenced this pull request Oct 29, 2018
* Add `.mjs` support back to webpack, because webpack was not the problem

* Continue toggling `.mjs` to `javascript/auto` mode

* Be more inline with the specification

* Bump old Node to 6
@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mjs files get imported as static files
5 participants