chevrotain icon indicating copy to clipboard operation
chevrotain copied to clipboard

chore: use rollup instead of webpack and esbuild

Open NaridaL opened this issue 2 years ago • 7 comments

NaridaL avatar Nov 03 '21 07:11 NaridaL

  1. w/o deps, commonjs
  2. w/o deps, es
  3. w/ deps, umd
  4. w/ deps, umd + minified
  5. w/ deps, es
  6. w/ deps, es + minified

We want all these combinations?

NaridaL avatar Nov 03 '21 16:11 NaridaL

Separating the bundling and compilations steps was done intentionally. I personally believe it allows greater flexibility dev flows.

I see the following issues:

Issues / Cons

Major: (not working correctly)

  • The bundle:watch does not trigger when changes are made in other packages in this mono-repo.
  • The bundle does not include the other packages in this mono-repo or external packages, so I don't expect it to work in a browser env. -Edit: only now I noticed you commented on the deps/no-deps earlier.

Minor (possible concerns)

  • The main and exports fields pointing to a bundled artifact may cause the following issues:
    • will not be possible to leverage combining ESM and HTTP2 for parallel async loading
      • https://betterprogramming.pub/2020-004-the-rollout-of-modules-is-complete-d25f04870284
    • Possibly make it more difficult for bundling tools to perform tree shaking (not sure on this...).
  • combining the compile step with the bundling tool, would make it difficult to switch to a tool such as esbuild which is written in Go and thus is not likely to integrate with JS plugins.
  • In regards to the ESM wrapper: This blog recommended to avoid double transpiling, due to state management issues. And I already encountered some state related bugs with Chevrotain.
    • https://redfin.engineering/node-modules-at-war-why-commonjs-and-es-modules-cant-get-along-9617135eeca1

Pros

  • The (un-minified bundle) now gets automated testing as it is loaded by tests via exports / main field.
  • Avoid issues with source maps due to multiple transformation steps.
    • This may not be a major issue, perhaps we should disable source maps completely for the bundles as I personally am unlikely to debug an issues that is not reproducible in a nodejs env, so maybe it does not matter if there are no source maps.

bd82 avatar Nov 03 '21 19:11 bd82

My initial impression is is that there are more cons to replacing the main / exports fields than advantages. And I think it may be best to just implement a drop in replacement to the current development flows and types of bundles. But this is open for discussion.

A possible hybrid approach would be to, combine bundle + tsc compile, but only for the web bundles. and keep the separate mono-repo aware tsc compile step for the entry points of exports and main. But maybe we would want additional tests for the bundles in that case.

Long Term

Comparing the configuration complexity of rollup versus esbuild does make suspect I would prefer in the long term to use esbuild (once it supports UMD output natively). Basically if we can get x10 faster bundling with only two extra scripts in the package.json, It seems better than having a whole extra config file to me 😄

bd82 avatar Nov 03 '21 20:11 bd82

The big pro here is that the ES bundle works correctly now: It does export { CstParser, EmbeddedActionsParser, ... } instead of const module = { CstParser, EmbeddedActionsParser }; export default module. I'm unclear what the advantage of the ES bundle over UMD in the current form is.

Additionally the amount of custom scripts is reduced, which is good for maintability.

NaridaL avatar Nov 03 '21 22:11 NaridaL

The big pro here is that the ES bundle works correctly now: It does export { CstParser, EmbeddedActionsParser, ... } instead of const module = { CstParser, EmbeddedActionsParser }; export default module. I'm unclear what the advantage of the ES bundle over UMD in the current form is.

I completely agree.

I just wonder if we should (in mid term):

  • move to only support ESM
  • wont need the esm-wrapper then
  • wont need UMD bundle.
  • can keep using esbuild and only produce a 1-2 bundles (instead of 4).
  • Maybe won't need any bundle if we use ESM and expect consumers to bundle themselves with tree-shaking?

See more on this topic here:

  • https://github.com/Chevrotain/chevrotain/issues/1698

bd82 avatar Nov 03 '21 23:11 bd82

Technical discussion on supporting dual style libraries (CJS/ESM).

  • https://github.com/node-fetch/node-fetch/pull/1227

bd82 avatar Nov 04 '21 02:11 bd82

@NaridaL I appreciate the effort you've put into this PR. 👍 However, it is possible we would be better off by shelving it and re-evaluating the status in ~6 months.

This in no way means this PR is "waste effort", rather it is quite possible the main benefit of this PR and other issue your raised regarding ESM bundling is in the discussion itself, deep dive into ESM vs CJS and possible future decisions in regards to ESM / dual CJS/ESM support.

My thoughts at the moment is that we may even be able to drop all bundled artifacts if we can switch to an ESM only output in the mid term. Which means both goals of:

  1. Reducing maintenance TCO.
  2. Better support for ESM.

Could be achieved at the cost of breaking CJS support.

The main question is how stable would the eco-system support for ESM be in the mid term? But as long as it does not take more than ~1 year to reach the state where CJS support can be dropped It might not be worth additional effort investment in the introduction of rollup.

WDYT?

bd82 avatar Nov 04 '21 02:11 bd82

I am closing the PR for now with the hope that in ~7 months when nodejs 14 reaches EOL we can provide an ESM only package, perhaps even without a bundling step so no bundling tool would be needed.

bd82 avatar Aug 13 '22 12:08 bd82