esbuild icon indicating copy to clipboard operation
esbuild copied to clipboard

Producing esm bundles: Externalized dependencies should always be imported

Open dzearing opened this issue 4 years ago • 5 comments

I'm bundling react directly (index.js) with format=esm, trying to produce a browser consumable bundle. The library is commonjs and depends on the package object-assign, which I want to externalize to another bundle (external: ['object-assign'] in the config.) When externalizing, the require statement is not removed from the bundle output:

var _assign = require('object-assign');

This breaks using the bundle in the browser, as require is not defined.

Is there any clever way using esbuild, or the plugin system, to translate this at build time to an esm import? At the top of the bundle, I would like to see this in the bundle output:

import objectAssign from 'object-assign';

And no leftover require statements, making it loadable by the browser in conjunction with an import-map resolving the object-assign dependency.

I got very close using a plugin, by making onResolve of the dependency resolve to a generated stub file which imports and re-exports the library. However, in doing so the commonjs assign reference being imported would resolve to the export map, rather than the default export. I don't know how to control that.

Note: I tried this using Rollup + the commonjs plugin. By default it externalizes the dependency and I see this at the top of the output:

import require$$0 from 'object-assign';

dzearing avatar Apr 13 '21 22:04 dzearing

Another thread about this: #566.

I got very close using a plugin, by making onResolve of the dependency resolve to a generated stub file which imports and re-exports the library. However, in doing so the commonjs assign reference being imported would resolve to the export map, rather than the default export. I don't know how to control that.

Can't you just access the default property to get the default export?

Note: I tried this using Rollup + the commonjs plugin. By default it externalizes the dependency and I see this at the top of the output:

This currently isn't done automatically because doing this unavoidably changes the semantics of the code (dynamic require does not run in the same order as static import), and I'm not sure that a bundler should silently change the semantics of your code.

evanw avatar Apr 15 '21 01:04 evanw

Definitely understand the concern in changing the order of operations. Side effects and initialization end up running in a different order when translating commonjs, given the differences with esm.

FWIW I think I would still expect the imports to be hoisted as the default behavior though; especially across package boundaries, the initialization order should probably not be a part of the contract. Perhaps with require statements inside the same package, you could have ordering issues. But I think if there are real scenarios where order of import execution matters, the author should be adjusting their file structures. But this seems like an edge case.

But given that changing order may cause unintended effects, lets say we do this in a plugin. How would this be done? All I can come up with is a pre-proccessor of sorts in an onLoad callback, which loads the source, finds all require statements, does a replacement of them with a variable, and injects imports at the top of the file. Sounds like it would add significant performance overhead.

dzearing avatar Jul 09 '21 19:07 dzearing

@evanw what would be the implications of changing the __toCommonJS wrapper to consider the default export as well. Instead of returning the exports on an empty object, returning it on the default export if the default export is a function ?

gb-jos avatar Oct 13 '22 13:10 gb-jos

@evanw ignore my comment above I got it working with the plugin proposed in #566 by changing the conversion module to be import mod from ${JSON.stringify(args.path)}; module.exports=mod

@dzearing seemingly if you adapt the solution with the plugin in #566 to return import mod from ${JSON.stringify(args.path)}; module.exports=mod instead of export * from ${JSON.stringify(args.path)} it would resolve the default instead of the import map

gb-jos avatar Oct 13 '22 14:10 gb-jos

@evanw This was opened a bit ago, and after thinking about this for some time and using Rollup for cases where I need to externalize libraries being required, i've observed that 99% of the time, we should be hoisting the require up to the top as an import. Yes, it does change order of operations, but typically this isn't a blocking issue, and when it is in that 1% of the time, the user would need to opt out by not externalizing the library.

This is what Rollup does, fwiw. In the project we work on, when we detect this scenario, we have to fall back to Rollup for the job because of this issue.

It would super great to add this feature, despite the inconsistency it might introduce.

dzearing avatar Jul 02 '24 21:07 dzearing