goatslacker / alt

Isomorphic flux implementation
http://alt.js.org/
3.45k stars 323 forks source link

Fix module exports. #608

Closed mareksuscak closed 8 years ago

mareksuscak commented 8 years ago

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 commented 8 years ago

Can you rebase this?

Also, what does this add?

mareksuscak commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

Only adds a single line for each module:

module.exports = exports['default'];
jdlehman commented 8 years ago

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 commented 8 years ago

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

jdlehman commented 8 years ago

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:

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 commented 8 years ago

@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 commented 8 years ago

@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 :+1:

taion commented 8 years ago

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 commented 8 years ago

Yeah this is ok to do for people using ES5.