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 ES6 module build #2530

Merged
merged 1 commit into from
Nov 20, 2015
Merged

Add ES6 module build #2530

merged 1 commit into from
Nov 20, 2015

Conversation

taion
Copy link
Contributor

@taion taion commented Nov 12, 2015

Closes #2524. Replaces #2523.

Don't merge this, it's just a strawman to show what we'd have to change.

@taion
Copy link
Contributor Author

taion commented Nov 15, 2015

@mjackson Thoughts on this? Do you still want to hold off on adding this to the build? Seems like both the benefits and the drawbacks are correspondingly minor.

"build": "rimraf lib && babel ./modules -d lib --ignore '__tests__'",
"build": "npm run build-cjs && npm run build-es6",
"build-cjs": "rimraf lib && babel ./modules -d lib --ignore '__tests__'",
"build-es6": "rimraf es6 && babel ./modules -d es6 --blacklist=es6.modules --ignore '__tests__'",
Copy link
Member

Choose a reason for hiding this comment

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

Where do you get the --blacklist flag from? I don't see it in the docs.

Copy link
Member

Choose a reason for hiding this comment

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

Basically, this is supposed to do all transpiling except for modules, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The docs on the page now are for Babel 6. This is a Babel 5 option to disable a specific transform, per https://github.com/babel/babel.github.io/blob/862b43db93e48762671267034a50c30c00e433e2/docs/usage/options.md#options.

This is just making a build that is applying all of our normal Babel transforms, except for the ES6 module transform, leaving the import/export statements intact. It even properly transpiles the ES7 export extensions we use on index.js to be vanilla ES6.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly. It will look a little different with Babel 6, since --blacklist is gone there, and everything goes through plugins and presets, but it would thematically be very similar.

@taion
Copy link
Contributor Author

taion commented Nov 19, 2015

This has been open for a week or so now - should we move forward with this change, or drop this for the time being?

On balance I think adding the ES6 module build adds enough value for our users to justify the additional configuration involved that this PR adds.

(Please don't merge as-is, though - at least not with the commit comment the way it is.)

@taion taion changed the title [STRAWMAN] [DO NOT MERGE] Add ES6 module build Add ES6 module build Nov 19, 2015
@taion
Copy link
Contributor Author

taion commented Nov 20, 2015

Should this actually be es2015/?

@taion
Copy link
Contributor Author

taion commented Nov 20, 2015

Going to merge this shortly unless it turns out there's a good reason to bundle this build.

taion added a commit that referenced this pull request Nov 20, 2015
@taion taion merged commit 6884ff1 into remix-run:master Nov 20, 2015
@taion taion deleted the es6-build branch November 20, 2015 17:16
@SimenB
Copy link

SimenB commented Nov 21, 2015

Should be noted perhaps that webpack doesn't look up jsnext:main by default?
webpack/webpack#1612 (comment)

@taion
Copy link
Contributor Author

taion commented Nov 21, 2015

Fair point, but if it's an upstream issue, I think it's okay to wait for a bit before addressing.

@mjackson
Copy link
Member

@taion Please get approval from either @ryanflorence or myself before merging stuff like this. Neither of us actually want an ES6 module build at this time.

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

Successfully merging this pull request may close these issues.

3 participants