add .mjs to list of well known extensions by bmeck · Pull Request #4691 · webpack/webpack

bmeck · 2017-04-11T15:23:09Z

What kind of change does this PR introduce?

feature, compliance w/ Node.js EP

Did you add tests for your changes?

No

If relevant, link to documentation update:

Summary

this is to conform to the plan of the Node.js EP.

Does this PR introduce a breaking change?

No

Other information

this is to conform to the plan of the Node.js EP

jsf-clabot · 2017-04-11T15:23:10Z

All committers have signed the CLA.

bmeck · 2017-04-11T15:23:31Z

I am unclear on how to ensure the extension is only ever an ES Module.

TheLarkInn · 2017-04-19T04:58:04Z

We could build the same kind of automatic loader support like we did for json and only just test for the extension, and thus turn off all the other parser features for those modules. Otherwise, I'm not sure if theres a really "GREAT" way to do it because we determine the module based on the lexical tokens used etc.

bmeck · 2017-04-19T16:24:00Z

@TheLarkInn I think only the target source type needs to change, are there other parser features that need to change?

loganfsmyth · 2017-05-01T21:58:19Z

Any thoughts on what you'd expect if someone runs babel-loader or any other loader on these and transforms the content to CommonJS before Webpack can get to it? Should we just let Webpack error about the usage of module.exports or something?

bmeck · 2017-05-01T22:03:53Z

well if webpack sees it is still a .mjs extension it would error. Is it possible to give it a new filename/specify the language goal when handing it off?

sokra

Is the order correct? Does it really prefer .js over .mjs?

It would be great to have some simple test here, which tests the preferences for extensions.

bmeck · 2017-05-02T06:58:43Z

@sokra I've left things unchanged in this PR, but .mjs would probably be preferred if that seems an ok breaking change. I will add tests for that as well.

bmeck · 2017-05-02T13:42:47Z

@loganfsmyth after some thought, ideally .mjs would become .js when being handed off.

loganfsmyth · 2017-05-02T16:10:30Z

@bmeck Yeah, that seems like the ideal to me too. I guess that will have to wait to see if Webpack could add logic to make the parse goal explicit.

webpack-bot · 2017-05-05T14:18:01Z

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

jhnns · 2017-05-09T16:40:27Z

 				return callback();
 			}

+			const isMJS = this._source._name && /.mjs$/.test(this._source._name);


/.mjs$/ should be /\.mjs$/ 😁

@sokra is there a common name property on all possible sources? Accessing the private _name property is not ideal here.

webpack-bot · 2017-05-09T19:32:51Z

@bmeck Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@jhnns Please review the new changes.

jhnns

Thanks @mbeck! 👍

As I already commented on the diff, passing an array as sourceType does not work. We need to change the Parser.prototype.parse implementation for that.

In the long term, I would like to achieve feature parity with Node.js, as this encourages to write code that works potentially in both environments (actually, we did it already with CJS).

Feature parity means for me:

.js must contain CJS
.mjs must contain ESM
More controversial: Importing "named exports" from CJS will raise an error

I think, the last one is more controversial since importing named exports does already work for us. However, I think it's more important to provide a consistent and unified environment. That's why I would deliberately remove this feature.

In the short term, the transition should be as painless as possible. For now, we should preserve the old behavior for .js files and just enforce the module parser goal for .mjs. This means that for .js files, the parser would still just try the module parser goal first and then back off to script. Importing named exports from CJS should still work for now.

Once we agreed on this, we should publish guidelines in order to write code that works in Node.js and Webpack.

Any thoughts @sokra @TheLarkInn @bebraw ?

jhnns · 2017-05-10T12:23:32Z

 		this.options = options;
 		options.type = options.type || "require";
-		options.extensions = options.extensions || ["", ".js"];
+		options.extensions = options.extensions || ["", ".mjs", ".js", ];


Is that trailing comma by accident? 😁
(It does no harm here, but we usually do not use trailing commas in one-liners)

jhnns · 2017-05-10T13:06:45Z

 			try {
-				this.parser.parse(this._source.source(), {
+				this.parser.parse(source, {
+					sourceTypes: sourceTypes,


Passing an array as sourceType is not supported. These options are not passed to acorn anyway (@sokra could you clarify the purpose of this second argument initialState)?

jhnns · 2017-05-10T13:09:18Z

@loganfsmyth in the long term, parsing a .mjs module that has been transformed to CJS should raise an error since this is clearly a misconfiguration of the babel-loader. Why would you want to transform ESM to CJS? 😁

bebraw · 2017-05-10T13:10:40Z

In the short term, the transition should be as painless as possible. For now, we should preserve the old behavior for .js files and just enforce the module parser goal for .mjs. This means that for .js files, the parser would still just try the module parser goal first and then back off to script. Importing named exports from CJS should still work for now.

Yeah, soft deprecation (a warning before dropping the feature) would be a good way to go. 👍

loganfsmyth · 2017-05-10T22:07:37Z

@jhnns One example would be if you still wanted to try to preserve the named exports behavior to ease the transition to Node's behavior, it could be nice to compile your es6 syntax with Babel, so the import would still behave like CommonJS. Or say there is some feature that is hard to compile to ES6 but is easy to do if you can convert it to CommonJS.

In a more general case, if I have a loader for some extension that isn't .js or .mjs, say one of the file loaders, if that wants to output ES6 instead of CommonJS, what will Webpack do? It seems like it would be good to have a way for loaders to assert which grammar they expect their output to be parsed as.

jhnns · 2017-05-11T09:34:48Z

One example would be if you still wanted to try to preserve the named exports behavior to ease the transition to Node's behavior, it could be nice to compile your es6 syntax with Babel, so the import would still behave like CommonJS.

Mhmm... I think, once one of your dependencies have been translated to ES modules, you will probably have a hard time with still using CommonJS because now you need to convert your synchronous require into an asynchronous import, like:

let someEs6Module;

require.import("some-es6-module").then(module => someEs6Module = module);

Actually, this means that every function inside your module that uses someEs6Module must be asynchronous. I don't see the possibility to make this painless...

You can ease the transition for people who are using bundlers by providing different entry points ("main" vs. "module"), but this doesn't apply to people who are just using Node.js. Or am I missing something?

I just realized that this change is more nuanced than I thought. If we want to have feature parity with Node.js, we also need to adapt this asynchronous import behavior from ES to CJS...

bmeck · 2017-05-11T14:30:12Z

@jhnns this is in part why in the EP https://github.com/bmeck/node-eps/blob/a1eab9bf023bbe13a79ddb18f0622a5d57215f9b/002-es-modules.md you can't require ESM

bmeck · 2017-05-11T14:31:02Z

this has grown in scope quite a bit and I probably don't have bandwidth to draw it to completion anytime soon if someone else wants to take it up

loganfsmyth · 2017-05-11T18:05:23Z

Mhmm... I think, once one of your dependencies have been translated to ES modules, you will probably have a hard time with still using CommonJS

That's a fair point, I forgot there were limitations there then.

Then ignoring that part, the other question for me is if a loader for a non .js/.mjs extension wants to output ESM, how will Webpack handle it? And separately, if there are other loaders in the chain, how should they decide which grammar to use?

bmeck · 2017-07-18T18:43:05Z

@sokra is there someone who could take this over since it expanded in scope so much?

mikesherov · 2017-07-18T19:28:29Z

 				return callback();
 			}

+			const isMJS = this._source._name && /\.mjs$/.test(this._source._name);


is Michael Jay Sherov

😂 whatever floats your 🐐

bmeck · 2017-08-16T17:00:39Z

w/e this becomes should not land until it can be forced to be a Module as per the new Internet Draft:

https://datatracker.ietf.org/submit/status/88872/

quantizor · 2017-09-22T22:48:06Z

@sokra can this be merged now that Node is shipping mjs support under a flag?

pierreneter · 2017-09-23T07:51:47Z

Why not let babel-loader handle it?

loganfsmyth · 2017-09-23T17:32:47Z

@pierreneter babel-loader does not change the internal config of Webpack itself, that is unrelated.

sokra · 2017-11-27T07:05:24Z

done on next branch

add .mjs to list of well known extensions

c568be6

this is to conform to the plan of the Node.js EP

webpack-bot added PR: unreviewed PR: small labels Apr 11, 2017

Rich-Harris mentioned this pull request Apr 11, 2017

add .mjs to list of well known extensions rollup/rollup#1364

Closed

webpack-bot added the PR: CI-ok label Apr 11, 2017

sokra requested changes May 2, 2017

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: unreviewed labels May 2, 2017

bmeck added 2 commits May 2, 2017 08:49

prefer .mjs to .js

2bee180

only use Module goal for .mjs

0e20600

webpack-bot added PR: review-outdated and removed PR: reviewed-changes-requested labels May 2, 2017

webpack-bot added PR: CI-not-ok and removed PR: CI-ok labels May 2, 2017

bmeck added 2 commits May 2, 2017 11:40

add tests for path resolution

6bfbe29

lint fix

e6de924

bmeck changed the title ~~[do not merge] add .mjs to list of well known extensions~~ add .mjs to list of well known extensions May 5, 2017

webpack-bot added PR: CI-ok and removed PR: CI-not-ok labels May 5, 2017

jhnns requested changes May 9, 2017

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: review-outdated labels May 9, 2017

fix regex

c73d062

webpack-bot added PR: review-outdated and removed PR: reviewed-changes-requested labels May 9, 2017

jhnns requested changes May 10, 2017

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: review-outdated labels May 10, 2017

mikesherov reviewed Jul 18, 2017

View reviewed changes

sokra added the PR: breaking change label Oct 11, 2017

sokra added this to the webpack 4 milestone Oct 11, 2017

sokra closed this Nov 27, 2017

SleeplessByte mentioned this pull request Sep 21, 2018

Build service worker for --esm preactjs/preact-cli#606

Merged

Uh oh!

Conversation

bmeck commented Apr 11, 2017

Uh oh!

jsf-clabot commented Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmeck commented Apr 11, 2017

Uh oh!

TheLarkInn commented Apr 19, 2017

Uh oh!

bmeck commented Apr 19, 2017

Uh oh!

loganfsmyth commented May 1, 2017

Uh oh!

bmeck commented May 1, 2017

Uh oh!

sokra left a comment

Choose a reason for hiding this comment

Uh oh!

bmeck commented May 2, 2017

Uh oh!

bmeck commented May 2, 2017

Uh oh!

loganfsmyth commented May 2, 2017

Uh oh!

webpack-bot commented May 5, 2017

Uh oh!

jhnns May 9, 2017

Choose a reason for hiding this comment

Uh oh!

webpack-bot commented May 9, 2017

Uh oh!

jhnns left a comment

Choose a reason for hiding this comment

Uh oh!

jhnns May 10, 2017

Choose a reason for hiding this comment

Uh oh!

jhnns May 10, 2017

Choose a reason for hiding this comment

Uh oh!

jhnns commented May 10, 2017

Uh oh!

bebraw commented May 10, 2017

Uh oh!

loganfsmyth commented May 10, 2017

Uh oh!

jhnns commented May 11, 2017

Uh oh!

bmeck commented May 11, 2017

Uh oh!

bmeck commented May 11, 2017

Uh oh!

loganfsmyth commented May 11, 2017

Uh oh!

bmeck commented Jul 18, 2017

Uh oh!

mikesherov Jul 18, 2017

Choose a reason for hiding this comment

Uh oh!

TheLarkInn Jul 19, 2017

Choose a reason for hiding this comment

Uh oh!

bmeck commented Aug 16, 2017

Uh oh!

quantizor commented Sep 22, 2017

Uh oh!

pierreneter commented Sep 23, 2017

Uh oh!

loganfsmyth commented Sep 23, 2017

Uh oh!

sokra commented Nov 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

jsf-clabot commented Apr 11, 2017 •

edited

Loading