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

Fix module exports. #608

Merged
merged 2 commits into from Jan 26, 2016
Merged

Fix module exports. #608

merged 2 commits into from Jan 26, 2016

Conversation

mareksuscak
Copy link
Contributor

Starting with Babel 6 and onwards, Babel no longer exports libraries under module.exports which is breaking our builds. There's a neat plugin that fixes the behavior.

@goatslacker
Copy link
Owner

Can you rebase this?

Also, what does this add?

@mareksuscak
Copy link
Contributor Author

@goatslacker Done. Explained here: http://stackoverflow.com/questions/34736771/webpack-umd-library-return-object-default/34778391#34778391

TL;DR: This is what Babel 5 was doing:

exports.default = Alt;
module.exports = exports['default'];

and this is what Babel 6 is doing by default

exports.default = Alt;

The plugin I have added fixes this so that the output is similar to what Babel 5 used to produce. It was breaking our builds since we're still using require.js and that would enforce us to refactor to require alt this way (breaks compatibility):

var Alt = require('alt').default;

instead of

var Alt = require('alt');

@goatslacker
Copy link
Owner

Yeah I know babel6 makes it spec compliant where it wasn't before. I'm wondering what the babel plugin does. Does it replace babel's export with its own? Does it just add a module.exports?

@mareksuscak
Copy link
Contributor Author

Only adds a single line for each module:

module.exports = exports['default'];

@jdlehman
Copy link
Collaborator

Wouldn't it make more sense to fix our exports to be Babel 6 compliant since that is the spec for ES6 modules anyway?

@mareksuscak
Copy link
Contributor Author

From my understanding it still will be spec compliant but will add a backwards compatible export.

@jdlehman
Copy link
Collaborator

Right, but the "backwards compatible export" is providing backwards compatibility with an old version of Babel, which incorrectly allowed people to go against the spec. In other words, this change adds backwards compatibility for a bug with Babel.

At the end of the day this needs to be fixed such that people using alt can still require alt without digging into a default property on the object being exported, as you stated above.

var Alt = require('alt');

To achieve this we have 2 options:

  • One way to do that would be to add this plugin, which would allows us to leave the code in alt as is.
  • The other way is to update the code in alt to export in a way that works with the ES6 spec.

Either change will not affect end-users of alt.

I'd be curious to hear if @goatslacker has an opinion on which way we should go forward. Both ways should require minimal changes and I believe option 2 makes more sense as it would still work when browsers reach enough support for ES6 that a Babel build is no longer necessary, while option 1 would be broken without a Babel build because it is not spec compliant.

@taion
Copy link
Collaborator

taion commented Jan 26, 2016

@jdlehman

Is there an explicit spec for how CJS and ES2015 exports should interact?

I feel like from a maintainer ergonomics perspective (not that I've checked in any code at all to this library), having to remember to export everything twice to allow both ES2015 and CJS imports seems error-prone.

Not having an easy way (without this plugin) to provide CJS-compatible exports without having to export everything twice has been one of the things preventing us from moving React Router and React-Bootstrap to Babel 6.

@jdlehman
Copy link
Collaborator

@taion Ah, I see the dilemma now. I guess this does make sense as we can't assume the tooling that an end-user is using, so this probably actually is a good intermediary step with build tools in the state they are in.

So I am now 👍

@taion
Copy link
Collaborator

taion commented Jan 26, 2016

The other option would be to just use module.exports to export things, but that makes the code look nasty IMO, since import is much nicer syntax than require().

@goatslacker
Copy link
Owner

Yeah this is ok to do for people using ES5.

goatslacker added a commit that referenced this pull request Jan 26, 2016
@goatslacker goatslacker merged commit 9c5a2d1 into goatslacker:master Jan 26, 2016
@mareksuscak mareksuscak deleted the fix-module-exports branch January 26, 2016 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants