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

CommonJS exports of objects with prototypes are handed differently from webpack #326

Closed
iamakulov opened this issue Aug 12, 2020 · 3 comments

Comments

@iamakulov
Copy link

Overview

Consider the following (simplified) code from a real codebase:

// module.ts
import { Path } from 'paper'
new Path()

// paper/dist/paper-core.js
class Path { }
module.exports = Object.create({
  Path,
})

The paper module is a CommonJS module that exports an object. The unusual thing about that object is that it doesn’t have its own properties – instead, all properties are inherited.

When webpack and ESBuild encounter such module, they seem to behave differently. Both webpack and ESBuild pass this CommonJS module through an interop – which converts

module.exports = someObject

to (roughly)

bundlerExportsSlot = {
  ...someObject,
  default: someObject,
}

However, unlike webpack, ESBuild ignores inherited properties. This means the following code:

import { Path } from 'paper'
new Path()

imports class Path with webpack but undefined with ESBuild. And so, with ESBuild, new Path() results in TypeError: Path is not a constructor.

Repro

  1. Clone https://github.com/iamakulov/esbuild-webpack-prototype-repro

  2. Run yarn

  3. Run the webpack build and verify that it resolves the named export properly:

    $ yarn webpack
    $ node dist/webpack-bundle.js
    {} Segment Segment
  4. Run the webpack build and verify that it doesn’t resolve the named export:

    $ yarn esbuild src/index.ts --bundle --outfile=dist/esbuild-bundle.js --platform=node
    $ node dist/esbuild-bundle.js
    {} Segment undefined
    
@evanw
Copy link
Owner

evanw commented Aug 12, 2020

Thank you for reporting this inconsistency. I did a quick survey of how different tools handle this. Here's the code I ran:

import Paper from "./paper.js";
import { Segment } from "./paper.js";
import * as star from "./paper.js";
console.log(Paper, Paper.Segment, Segment, star);

And the results:

  • Using node with --experimental-modules

    import { Segment } from "./paper.js";
             ^^^^^^^
    SyntaxError: The requested module './paper.js' does not provide an export named 'Segment'
    
  • Using tsc with --esModuleInterop

    {} Segment Segment ['default']
    
  • Webpack with ts-loader

    {} Segment Segment []
    
  • Rollup with @rollup/plugin-commonjs

    {} Segment Segment ['default', '__moduleExports']
    
  • Parcel

    {} Segment undefined ['default']
    
  • Using esbuild with --bundle

    {} Segment undefined ['default']
    

Everything does something different with the exception that esbuild matches Parcel. It looks like none of them consider inherited properties as exports for star imports. Right now regular imports and star imports behave consistently in esbuild, so I think addressing this issue while maintaining compatibility with star imports means making regular imports and star imports no longer consistent with each other.

Edit: Actually, I've thought about this more and I changed my mind. I think it is appropriate for esbuild to break compatibility with star exports. Ideally either something is exported or it isn't. If we're going to consider prototype properties as exports, then that should be treated consistently in all cases. So I now believe the correct output should be this:

{} Segment Segment ['default', 'Segment', 'Path']

@evanw
Copy link
Owner

evanw commented Aug 13, 2020

This was just released in version 0.6.22.

@idevelop
Copy link

@evanw I just came across this inconsistency in our project, where we recently (very happily) switched form webpack to esbuild for insanely huge build time improvements (awesome work btw!).

We use sequelize, which exports the Sequelize class as default, and some of the functions it exports, for example literal() are:

  • declared in its type definition as being top-level exports, so typescript lets you write import { literal } from 'sequelize'
  • actually defined on Sequelize.prototype in the module js code

Because of that, the exportStar never even iterates through this literal property.

An interesting thing to note here is that literal is also defined as a static function on class Sequelize, so I think that even without being defined on Sequelize.prototype, it should be part of the exported namespace, but then for (let key in module2) in exportStar does not seem to iterate over it. I wonder if replacing that for with Object.getOwnPropertyNames(module2) might resolve this.

For now I've rewritten our import to work around this, but it's concerning that this only manifests at runtime, as it makes me worry that we have other such imports from other libraries.

Curious if you have any advice on how we should prevent this.

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

3 participants