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

Kill CommonJS default export behaviour #2212

Closed
sebmck opened this issue Aug 14, 2015 · 24 comments
Closed

Kill CommonJS default export behaviour #2212

sebmck opened this issue Aug 14, 2015 · 24 comments
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Milestone

Comments

@sebmck
Copy link
Contributor

sebmck commented Aug 14, 2015

No description provided.

@sebmck sebmck added this to the 6.0.0 milestone Aug 14, 2015
@cesarandreu
Copy link
Contributor

What's the planned behavior going forward?

@sebmck
Copy link
Contributor Author

sebmck commented Aug 14, 2015

@cesarandreu To always export a default to exports.default.

@jmm
Copy link
Member

jmm commented Aug 14, 2015

@sebmck Is this in reference to the issue discussed in #2047? I've been finding myself agreeing with
@UltCombo that that behavior can be a headache.

@UltCombo
Copy link

👍
Adding .default to your CJS require()s can be slightly bothersome, but not nearly as much as having an unexpected breaking change due to simply adding a named export to the imported module.

@sebmck
Copy link
Contributor Author

sebmck commented Aug 14, 2015

@jmm Not really. It's the fact that people are (wrongly) doing:

a.js

import { foo, bar } from "./b";

b.js

export default {
  foo: "foo",
  bar: "bar"
};

And Babel is letting them.

@UltCombo
Copy link

@sebmck That's just another symptom of bad implicit magic, imho. If you add a named export to b.js then it will be a breaking change to a.js (although it should be broken since the beginning).

@jmm
Copy link
Member

jmm commented Aug 14, 2015

@sebmck Oh ok, I wasn't aware of that behavior. 👍 to killing that off (obviously).

@jquense
Copy link
Contributor

jquense commented Aug 16, 2015

Hi! first, thanks for all the hard work and excellent tool. is the idea that commonJS consumers would need to require().default for all modules compiled with Babel? Reduced magic is nice, but that seems like it would be a deal breaker for folks publishing to the node ecosystem where such a convention doesn't exist. Perhaps it's worth it but if it's helpful feedback, I think if that was the behavior I'd stop using the module syntax rather then ask consumers to use .default.

@UltCombo
Copy link

@jquense or you can tell the consumers to prefer the ES2015 module syntax over the CJS one. 😉

@jquense
Copy link
Contributor

jquense commented Aug 16, 2015

@UltCombo, I really can't tell anyone building a node app to also use Babel to transpile their app to avoid some non idiomatic import with my module. they just won't use my module instead

@UltCombo
Copy link

@jquense I don't really see a problem as long as the API usage is properly documented for CJS and ES Modules. You may as well use a named export to give a more meaningful property name than default.

@jquense
Copy link
Contributor

jquense commented Aug 16, 2015

It's just not idiomatic, it breaks expectations and is a hassle (albeit a small one) for consumers. for packages that provide a lot of modules, asking ppl to remember the name of a bunch of single export module names is an unnecessary burden. ultimately I am not going to over complicate an API surface just so I can use the es6 module syntax. I appreciate that others are willing to make that tradeoff, I just wanted to provide feedback that I would not, hoping that it provides a helpful perspective :)

@UltCombo
Copy link

If you really want to use ES Modules syntax without having to ask your CJS consumers to add .default after the require() call, you can re-export it in your package's entry point:

module.exports = require('./transpiled/index').default;

@jmm
Copy link
Member

jmm commented Aug 17, 2015

@UltCombo @jquense Since I was mistaken about the subject of this issue, how about we continue discussion of the module.exports = exports["default"] behavior in #2047.

@UltCombo
Copy link

@jmm that sounds good to me. But either way, once this issue (#2212) is resolved, the module.exports = exports["default"] will no longer exist and thus "fix" #2047 in the way I suggested, no?
/cc @sebmck

@jmm
Copy link
Member

jmm commented Aug 17, 2015

@UltCombo Hmm, I guess you're probably right that I didn't think that through and misunderstood the motivation, not the actual change. What a tricky little piece of interop. I somewhat agree that documenting the API of a module as require(whatever).default isn't a big deal. Idiomatic shouldn't be taken to the extreme of being dogmatic. And even in the node / iojs documentation they use examples like:

var Readable = require('stream').Readable;

That being said, I think most of the time, especially when the default export is a function (and even when there are other exports), I'd be pretty happy with output like:

exports["default"] = function () {};

module.exports = exports["default"];

module.exports["default"] = module.exports;

(Not saying that should be the default output, but I'd probably use it...unless someone points out bad problems with it I haven't thought of.) Then you could do require(whatever) or require(whatever).default. It allows misuse like sebmck showed, but that might just be one of those things where it "works if you play by the rules", otherwise you're on your own. For example, if my module isn't documented as exporting call then a consumer shouldn't be doing import {call}.

@jquense
Copy link
Contributor

jquense commented Aug 17, 2015

since esModules are being tagged with __esModule wouldn't it be possible just to make import { foo } from an es module with no named exports a compile or runtime error? There seems like there are other ways to solve this problem.

I agree that sticking to idioms isn't something that needs to be done dogmatically. However, getting folks onboard and using es modules over deeply established and working commonjs modules, is something transpilers can be very helpful in (and something that is needed if they are going to get used). However, I wouldn't underestimate folks not wanting to switch when it means adding a bunch of unnecessary api noise.

the reexport approach works fine if you are only exporting a single module, but its not weird to have many export modules, like lodash: require('lodash/object/pick') reexporting to exports in those cases amounts to only using es6 modules for importing, which is kinda lame. Its also like UMD round 2 which no one wants.

@UltCombo
Copy link

That being said, I think most of the time, especially when the default export is a function (and even when there are other exports), I'd be pretty happy with output like:

exports["default"] = function () {};

module.exports = exports["default"];

module.exports["default"] = module.exports;

I believe Babel has opted out of this way in the past due to conflicts in these cases:

export default { x: 1 };
export const x = 2;

Also cases such as iterating over the default export's properties, etc.

@jmm
Copy link
Member

jmm commented Aug 25, 2015

@UltCombo

I believe Babel has opted out of this way in the past due to conflicts in these cases:
...
Also cases such as iterating over the default export's properties, etc.

Yeah, that all makes sense. What I'm talking about is perhaps too opinionated as a default, but I think I'd like to be able to opt-in to doing what I suggested when the default export can be statically determined to be a function. And I'm guessing that's probably the 80% case for npm modules?

@jquense

since esModules are being tagged with __esModule wouldn't it be possible just to make import { foo } from an es module with no named exports a compile or runtime error? There seems like there are other ways to solve this problem.

Not compile-time -- each file is transformed in isolation. Maybe at run-time (with obvious, though maybe negligible, performance overhead)? How...exporting another property like __esModule that enumerates the named exports? I'm just thinking out loud here, I don't know if there's anything to the idea.

I agree that sticking to idioms isn't something that needs to be done dogmatically. However, getting folks onboard and using es modules over deeply established and working commonjs modules, is something transpilers can be very helpful in (and something that is needed if they are going to get used). However, I wouldn't underestimate folks not wanting to switch when it means adding a bunch of unnecessary api noise.

As you can see from my comments, I actually like the idea of supporting the common case better, at least in opt-in fashion, if there's a reasonable way to do it.

@UltCombo
Copy link

I'd suggest removing the default export behavior, then allow opting-in to it through loose: ["commonjs"] or similar.

@jmm
Copy link
Member

jmm commented Sep 2, 2015

So per discussion on Slack, apparently module formatters will be able to be implemented as plugins in v6. I'll probably look into creating a module formatter that behaves the way I suggested that can be opted in to.

@benlesh
Copy link

benlesh commented Sep 2, 2015

can we at least support it with a --flag of some sort?

@sebmck
Copy link
Contributor Author

sebmck commented Sep 28, 2015

Killed it. I'm not willing to support this in Babel, even as an option, as it's proved to be very dangerous for refactoring and semantics. Just to be clear, this just changes the export interop. Not the import.

@sebmck sebmck closed this as completed Sep 28, 2015
@babel babel locked and limited conversation to collaborators Sep 28, 2015
@babel-bot
Copy link
Collaborator

Comment originally made by @rymohr on 2015-12-16T02:58:03.000Z

This change also affects the common import-and-call pattern using require:

// was
require("./plugins/foo")(core);

// now
require("./plugins/foo").default(core);

I can see the argument against letting people shoot themselves in the foot with named imports, but this change definitely shits in the commonjs interop punchbowl.


Comment originally made by @rymohr on 2015-12-16T03:05:41.000Z

It also requires any coffee-script files that import ES6 modules to be refactored, fwiw. (for default exports at least)


Comment originally made by @rymohr on 2015-12-16T22:12:29.000Z

Came across this plugin if anyone needs to restore the babel 5 behavior: https://www.npmjs.com/package/babel-plugin-add-module-exports

@hzoo hzoo added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jul 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Projects
None yet
Development

No branches or pull requests

8 participants