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

ESM Bundle Improvements #1698

Closed
NaridaL opened this issue Nov 2, 2021 · 8 comments
Closed

ESM Bundle Improvements #1698

NaridaL opened this issue Nov 2, 2021 · 8 comments

Comments

@NaridaL
Copy link
Collaborator

NaridaL commented Nov 2, 2021

I haven't actually tested it, but the current ESM build looks pretty broken: https://unpkg.com/browse/chevrotain@9.1.0/lib_esm/chevrotain.mjs

It makes references to a comonjs function and only has one export. Tbh it looks like it's just exporting the commonjs module as the default export.

bundle:esm:regular should probably use the TS files directly as input, and not the webpack output.

Related to #1683

Related to #1697 because having a proper ES bundle would allow tree-shaking, for example for the generating-diagrams function, without requiring it to be extracted into a separate package.

@bd82
Copy link
Member

bd82 commented Nov 2, 2021

I haven't actually tested it, but the current ESM build looks pretty broken:

I don't think its broken as some people are using it afaik and opened issues at times it was actually broken.

It makes references to a comonjs function

It seems like some kind of wrapper ESBuild does, not a real commonjs function

and only has one export. Tbh it looks like it's just exporting the commonjs module as the default export.

Does not that make sense from a library perspective to export everything in the public API?

bundle:esm:regular should probably use the TS files directly as input, and not the webpack output.

Its not using the webpack output, but rather the TSC output.

Related to #1697 because having a proper ES bundle would allow tree-shaking, for example for the generating-diagrams function, without requiring it to be extracted into a separate package.

I am not sure if this is related, a library bundle must include everything, I don't expect this bundle to be used when a consumer of this library invokes a bundling process on their application.

@bd82
Copy link
Member

bd82 commented Nov 2, 2021

In regards to tree shaking:
I followed this blog to create the ESM wrapper.

I wonder if this wrapper would conflict with tree-shaking or not

@bd82 bd82 changed the title Broken ES Bundle ES Bundle Improvements Nov 2, 2021
@NaridaL
Copy link
Collaborator Author

NaridaL commented Nov 3, 2021

Yeah sorry, I jumped the gun a bit there and mixed up the bundle and not bundle. But there is the issue that the ES export (bundle or no bundle) exports a single big object as default export. This breaks tree-shaking for consumers.

In any case, it should be fixed by #1701 .

@bd82
Copy link
Member

bd82 commented Nov 3, 2021

But there is the issue that the ES export (bundle or no bundle) exports a single big object as default export. This breaks tree-shaking for consumers.

This is very interesting, how do we avoid multiple compilations and multiple package states (which esm-wrapper does)
while also allowing tree-shaking.

@bd82
Copy link
Member

bd82 commented Nov 3, 2021

Maybe a solution similar to lodash's style import Lexer from "chevrotain/Lexer" would meet all the requirements ?

@bd82 bd82 changed the title ES Bundle Improvements ESM Bundle Improvements Nov 3, 2021
@bd82
Copy link
Member

bd82 commented Nov 3, 2021

Another thing to consider is that all modern versions of nodejs support ESM (ECMAScript modules now).
While there are still compatibility issues around commonjs vs ESM, maybe most of those will be resolved in the mid term
and we can then only target ESM.

That will remove the usage of the esm-wrapper and reduce the number of bundled artifacts by 50%.

So maybe we should not invest much effort currently into this topic, and re-evaluate in ~6 months (nodejs 12 will even be deprecated then) depending on the state of the eco-system.

You can find a huge discussion on this here:

I wonder, we only ESM is supported, do we still need a UMD bundle? and if not we can stick with the fastest and simplest bundler to use (imho) esbuild

@bd82
Copy link
Member

bd82 commented Nov 3, 2021

node-fetch moved to esm only

Which caused a huge outcry 😄

@bd82
Copy link
Member

bd82 commented Mar 6, 2022

Closing this in favor of

@bd82 bd82 closed this as completed Mar 6, 2022
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