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

Make let default work even better #1987

Closed
chenglou opened this issue Sep 7, 2017 · 14 comments
Closed

Make let default work even better #1987

chenglou opened this issue Sep 7, 2017 · 14 comments

Comments

@chenglou
Copy link
Member

chenglou commented Sep 7, 2017

So apparently aside from let default = ... to export a default to be imported by babel/webpack through es6, we also need let __esModule = {value: true} also exported. I'm guessing this is how the es6 import of babel and such detect es6 modules?

cc @JasonRose

@anmonteiro
Copy link
Contributor

this looks too much like an implementation detail. I wonder if there's an alternative

@sansthesis
Copy link
Contributor

Yes, https://babeljs.io/docs/plugins/transform-es2015-modules-commonjs/ describes the transform. microsoft/TypeScript#3586 is where typescript added support (although they've had some drama about it recently).

@bobzhang
Copy link
Member

bobzhang commented Sep 8, 2017

@chenglou did you hit this case in practice

@chenglou
Copy link
Member Author

chenglou commented Sep 8, 2017

Jason did (at Instagram). Their setup is rather idiomatic webpack/babel I think?

@bobzhang
Copy link
Member

bobzhang commented Sep 8, 2017

So only when you use default export you need mark such property? Is this property a standard or just convention used by babel?

@IwanKaramazow
Copy link

It's something very specific to Babel as far as I know.

@glennsl
Copy link
Contributor

glennsl commented Sep 8, 2017

I've hit it. Real-life example here: https://github.com/glennsl/reasonable-gatsby-starter/blob/master/src/layouts/layouts_index.re#L73. Gatsby uses Babel to transpile es6 to es5 before bundling with webpack. So while I could avoid this by using es6 package-specs, it needs to be set to emit commonjs for webpack to be able to pick up the dependencies, since they aren't transpiled.

I've also seen several others needing to do this on Discord.

@bobzhang
Copy link
Member

bobzhang commented Sep 8, 2017

so, if we detect default exported we instrument let __esModule = {value: true}, will that work for you? or shall we instrument let _esModule for every module

@sansthesis
Copy link
Contributor

sansthesis commented Sep 9, 2017

I could be wrong, but I think that it is only exercised in the context of default imports. http://2ality.com/2015/12/babel-commonjs.html#default-imports is a code snippet that shows what gets generated when someone tries to do a default import with babel. So without knowing more, I think that it makes sense to only include the code when bsc would emit exports.default = ...;

@bobzhang
Copy link
Member

@JasonRose we can move forward with it, do you have a minimal way to confirm that intrument Object.defineProperty(exports, "__esModule", { value: true }); indeed solves the problem, otherwise broken. Thanks

@bobzhang
Copy link
Member

For people who are interested, here is my discovery, (apparently it is a babel-specific issue)

import foo from "foo"

var u = foo
var _foo = require("foo");

var _foo2 = _interopRequireDefault(_foo);

function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }

var u = _foo2.default;
export default foo
Object.defineProperty(exports, "__esModule", {
  value: true
});
exports.default = foo;

I think adding

exports.__esModule = true 

should be good enough, let me know what you think? IIRC, Object.defineProperty is terribly inefficient

@andreypopp
Copy link

@sansthesis
Copy link
Contributor

I believe that they use Object.defineProperty to make it not appear when enumerating keys on the exports object, which could cause issues.

@bobzhang
Copy link
Member

fixed in #2002 note that I did not use Object.defineProperty since exports is not designed to be enumerated on JS side, let me know if it works for you

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

No branches or pull requests

7 participants