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

Static methods from classes exported as default are not made available by __exportStar #472

Closed
idevelop opened this issue Oct 19, 2020 · 3 comments

Comments

@idevelop
Copy link

idevelop commented Oct 19, 2020

// hey.js

class Hey {
  static hello() {
    console.log("hello");
  }
}

module.exports = Hey;
// index.ts

import { hello } from "./hey";

hello();

esbuild index.ts --bundle produces javascript which, when executed, throws this error:

TypeError: hey.hello is not a function

This appears to be because esbuild passes the hey module through the __toModule function, which fails to identify the static hello() function as a valid function exposed on the module namespace, because for (let key in module) in __exportStar will not iterate over hello as a key of the module.

I wonder if this is considered a bug in esbuild, and if so, whether iterating over Object.getOwnPropertyNames(module) instead might be a valid solution.

@evanw
Copy link
Owner

evanw commented Oct 19, 2020

This is because the hello property is non-enumerable. I have tried to follow the same semantics as other bundlers but it definitely doesn't match the behavior of other bundlers as you can see here. The reason is because esbuild replicates their behavior in the dynamic binding case and applies that in all cases for consistency.

However, other bundlers are inconsistent in which exports work in which cases (named vs. star import, also static vs. dynamic binding). I consider this a bug in other bundlers (all other bundlers have this problem). I'm documenting this behavior in the examples below. I have used ns[Math.random() < 2 && 'name'] to test for dynamic property accesses since some bundlers treat ns.name and ns['name'] as identical expressions.

For default:

esbuild Parcel Webpack Rollup
import _default from './hey'
console.log(_default)
import _default, {hello} from './hey'
console.log(_default, hello)
import * as ns from './hey'
console.log(ns.default)
import * as ns from './hey'
console.log(ns[Math.random() < 2 && 'default'])

For hello:

esbuild Parcel Webpack Rollup
import {hello} from './hey'
console.log(hello)
import _default, {hello} from './hey'
console.log(_default, hello)
import * as ns from './hey'
console.log(ns.hello)
import * as ns from './hey'
console.log(ns[Math.random() < 2 && 'hello'])

Just changing to Object.getOwnPropertyNames doesn't work because people also expect that properties on the prototype chain are also exported (e.g. #326). One solution could be to both use Object.getOwnPropertyNames and walk up the prototype chain manually I suppose.

Changing this would also have the undesirable side effect of adding lots of extra properties to the exports object which may confuse people. Most objects have the Object prototype in their prototype chain so if we change this, logging the export object will suddenly show all sorts of random stuff like __defineGetter__. Addressing that could be done by copying over the enumerable property from the property descriptor instead of just using a normal property assignment.

I suspect trying to address all of this will have a noticeable performance penalty on the startup time of the generated modules. This is unfortunate but I'm not sure what to do about that.

@evanw evanw closed this as completed in fdf6824 Oct 20, 2020
@idevelop
Copy link
Author

Thanks for going into so much detail, I hadn't realised there would be so much inconsistency with imports.

My mental model is that import { foo } from 'module' is basically destructuring applied to the module's default export. I know that's probably an oversimplification or even wrong.

One interesting nuance is that, if the entrypoint is plain javascript instead of typescript:

// index.js

const { hello } = require("./hey");

hello();

then, when built with esbuild index.js --bundle --platform=node --external:./hey, the resulting code doesn't have any exportStar logic, so I'm wondering what value does piping the result of require() through exportStar offer, or what the nuance here is between js and ts, and whether the answer to that is different when --platform is node vs browser.

@evanw
Copy link
Owner

evanw commented Oct 22, 2020

My mental model is that import { foo } from 'module' is basically destructuring applied to the module's default export. I know that's probably an oversimplification or even wrong.

Sort of, although not in the pure ECMAScript module world. The default export is just another export name, albeit with special syntactic sugar. These two statements are equivalent:

import foo from 'module'
import { default as foo } from 'module'

Doing this does not get the property foo from the export named default. Instead it gets the export named foo:

import { foo } from 'module'

In case it helps: you can import both default and foo like this, which should hopefully show that both default and foo are just normal export names without any relation to each other:

import { default as a, foo as b } from 'module'

What's happening here is that esbuild is basically doing the following conversion for you to adapt this CommonJS module to ECMAScript:

module.exports = Hey
export default Hey
export const hello = Hey.hello

This results in two export names, default and hello.

what value does piping the result of require() through exportStar offer

Running the export through __exportStar does a few things:

  • It generates the default export and sets it equal to module.exports which is standard for CommonJS/ECMAScript module interop
  • It generates the __esModule export which tells Babel-like tools that this import object can be considered converted to ECMAScript module semantics
  • It encodes the exports as getters which means they are live (they update if the value on module.exports changes)
  • It ensures that the exports on this import object cannot be overwritten, which is required to match ECMAScript module semantics

If you prefer require semantics over import semantics, using require() like that instead should be perfectly fine since esbuild supports both paradigms.

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

No branches or pull requests

2 participants