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

Support for hybrid packages cjs/esm #840

Closed
dubzzz opened this issue Feb 18, 2021 · 8 comments
Closed

Support for hybrid packages cjs/esm #840

dubzzz opened this issue Feb 18, 2021 · 8 comments

Comments

@dubzzz
Copy link

dubzzz commented Feb 18, 2021

I recently wanted to check how esbuild would deal with hybrid packages. By hybrid I mean packages handling both CommonJS and ES Modules at the same time.

And it seems that the behaviour diverges from other bundlers and also from node when using platform=node.


Here is what I tried to do:

I took fast-check as an example of such package and tried to run the following snippet:

import fc from 'fast-check'
console.log(fc.__type) // prints "commonjs" if cjs version is used, "module" otherwise

Content of main.js* (*main.mjs for the case '.mjs extension')

Here are the results I got with multiple bundlers (including esbuild):

Setup Version imported Link repro*
node (.mjs extension) module link
node (type=module) module link
esbuild (platform=browser) module link
esbuild (platform=node) commonjs ⚠️ link
rollup module link
webpack module link

*In order to run the repro locally, you have to run: yarn && yarn start && echo out.txt

It seems that contrary to other setups, the one based on esbuild with platform=node imports the commonjs version.

My tests have been executed against [email protected] and [email protected].


I don't exactly know if it should be considered as expected or not, so I open the issue just in case it could help.

@osdevisnot
Copy link
Contributor

I suspect this is a config issue. @dubzzz can you try passing --format=esm when you run esbuild?

By default esbuild will set module to cjs when building for mode platform as documented here: https://esbuild.github.io/api/#format

@dubzzz
Copy link
Author

dubzzz commented Feb 18, 2021

Oh, thanks a lot. I'll definitely retry with it. I'll let you know.

I was expecting to have the same behaviour as the others by default without any specific option but perfect if it works.

@dubzzz
Copy link
Author

dubzzz commented Feb 18, 2021

@osdevisnot It seems to give the exact same result when I use:

--- esbuild main.js --bundle --platform=node --outfile=dist/main.node.js
+++ esbuild main.js --bundle --platform=node --format=esm --outfile=dist/main.node.js

The build still imports the commonjs version.

@evanw
Copy link
Owner

evanw commented Feb 18, 2021

This is likely deliberate behavior instead of a bug. There are some differences between the browser and node platforms for compatibility. From the documentation: https://esbuild.github.io/api/#platform:

When the platform is set to node: The main fields setting is set to main,module. This means tree shaking will likely not happen for packages that provide both module and main since tree shaking works with ECMAScript modules but not with CommonJS modules.

Unfortunately some packages incorrectly treat module as meaning "browser code" instead of "ECMAScript module code" so this default behavior is required for compatibility. You can manually configure the main fields setting to module,main if you want to enable tree shaking and know it is safe to do so.

Here's the issue that resulted in this change, with a lot of discussion: #363.

@dubzzz
Copy link
Author

dubzzz commented Feb 18, 2021

Thanks a lot for the quick response. Perfect 👌

I'm closing the issue

@dubzzz dubzzz closed this as completed Feb 18, 2021
@dubzzz
Copy link
Author

dubzzz commented Feb 18, 2021

@evanw Does esbuild take into account exports field when using platform=node? If so, it should have used the module version and not the commonjs one 🤔

@evanw
Copy link
Owner

evanw commented Feb 19, 2021

No it doesn’t yet. I still need to add that but given that the exports field can break working code, I’m waiting until the next breaking change release to implement that. This is tracked by #187.

@dubzzz
Copy link
Author

dubzzz commented Feb 19, 2021

Oh great! Thanks a lot your answers. I'll track the issue just to follow next moves

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