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

Rename ESM output to .mjs #1131

Merged
merged 2 commits into from
Sep 5, 2017
Merged

Rename ESM output to .mjs #1131

merged 2 commits into from
Sep 5, 2017

Conversation

geelen
Copy link
Contributor

@geelen geelen commented Aug 14, 2017

I've just been playing around with the @std/esm loader, which makes reference to the fact that Node's ES6 module support is going to be tied to the file extension *.mjs. I admit I don't know much context around this issue but I saw this and thought it might make sense to follow the new convention.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.682% when pulling 4b6fb9b on geelen:patch-1 into 3d47b59 on mobxjs:master.

@mweststrate
Copy link
Member

@rossipedia could you review this one?

@rossipedia
Copy link
Contributor

@geelen there's another minor change you'll need to make: pkg.module would also need to change to mobx.mjs.

I'm not 100% on the ramifications of this change at the moment, either. While ideally moving to .mjs is the Right Thing™ to do, and at least Webpack handles that extension currently, I'm not sure how many other bundlers / ecosystems would get tripped up by this. Although, to be fair, we gotta start somewhere. As far as my personal opinion goes, though, I'm all for it 👍

@capaj
Copy link
Member

capaj commented Sep 1, 2017

I personally dislike mjs, but it is the standard for node.js and browsers don't really care for it. So makes sense to have mjs instead of js.
IMHO all other bundlers should be allright, but people could run into issues if they have the file whitelisted/blacklisted somewhere. Would be best to release in 3.3.0 instead of in 3.2.3

@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 94.686% when pulling 3da0318 on geelen:patch-1 into 3d47b59 on mobxjs:master.

@mweststrate
Copy link
Member

Thanks & merged. Will be part of next release. Probably blowing up in somebodies face like any module / bundling related change ever did in the past 😢

@mweststrate mweststrate merged commit 953710b into mobxjs:master Sep 5, 2017
@mweststrate
Copy link
Member

Can somebody confirm this will work on React Native as expected as well?

@mweststrate
Copy link
Member

React native does not support .mjs extension yet. Also, webpack doesn't support it yet (will be added in 4). See: webpack/webpack#5686

Rolled back this PR and created a fresh follow up issue: #1172

@geelen
Copy link
Contributor Author

geelen commented Sep 20, 2017

Also, since I posted this PR, Node has gotten experimental support for modules which... I don't think should change things but who knows!

Probably blowing up in somebodies face like any module / bundling related change ever did in the past

That's my experience too. Good luck!

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

Successfully merging this pull request may close these issues.

5 participants