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

Producing esm bundles: Externalized dependencies should always be imported #1148

Open
dzearing opened this issue Apr 13, 2021 · 4 comments
Open

Comments

@dzearing
Copy link

dzearing commented Apr 13, 2021

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 dzearing changed the title Externalized commonjs imports using ESM format should be imported, not required. Producing esm bundle: Externalized dependencies should be imported, not required Apr 13, 2021
@dzearing dzearing changed the title Producing esm bundle: Externalized dependencies should be imported, not required Producing esm bundles: Externalized dependencies should be imported, not required Apr 13, 2021
@dzearing dzearing changed the title Producing esm bundles: Externalized dependencies should be imported, not required Producing esm bundles: Externalized dependencies should always be imported Apr 13, 2021
@evanw
Copy link
Owner

evanw commented Apr 15, 2021

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.

@dzearing
Copy link
Author

dzearing commented Jul 9, 2021

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.

@jiby-aurum
Copy link

jiby-aurum commented Oct 13, 2022

@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 ?

@jiby-aurum
Copy link

@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

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