chore: use rollup instead of webpack and esbuild #1701

NaridaL · 2021-11-03T07:22:27Z

No description provided.

NaridaL · 2021-11-03T16:28:00Z

w/o deps, commonjs
w/o deps, es
w/ deps, umd
w/ deps, umd + minified
w/ deps, es
w/ deps, es + minified

We want all these combinations?

NaridaL · 2021-11-03T16:51:24Z

packages/chevrotain/.mocharc.js

  reporter: "spec",
-  spec: "./lib/test/**/*spec.js"
+  spec: "./test/**/*spec.ts"


The "build tests and source first, then run built tests" seemed a bit awkward. Also it had problems with wrong source lines in stack traces.

This runs the .ts spec files directly on the .ts source. This should have the advantage that the coverage report is more useful as it applies to the the .ts source. Also fix-coverage-report shouldn't be necessary anymore (?)

bd82 · 2021-11-03T19:52:57Z

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 · 2021-11-03T19:56:29Z

packages/chevrotain/package.json

    "sinon": "11.1.2",
    "sinon-chai": "3.7.0",
-    "webpack": "5.59.1",
-    "webpack-cli": "4.9.1",
+    "ts-node": "^10.4.0",


I think these deps are no longer needed (from previous version with mocha + tsnode)

bd82 · 2021-11-03T20:01:43Z

packages/chevrotain/rollup.config.js

+      name: "chevrotain",
+      exports: "named"
+    })),
+    external: Object.keys(pkg.dependencies),


Should not we include everything in the bundle?

bd82 · 2021-11-03T20:02:29Z

packages/chevrotain/rollup.config.js

+  {
+    input: "src/api.ts",
+    output: [
+      ["es", false, true],


what do these combination mean?

bd82 · 2021-11-03T20:05:30Z

packages/chevrotain/rollup.config.js

+            terser({
+              compress: {
+                passes: 3,
+                unsafe: true,


How do we know everything works after bundling particularly when using unsafe minification modes? what benefit does unsafe produce in the bundle size?

bd82 · 2021-11-03T20:07:14Z

packages/chevrotain/rollup.config.js

+              compress: {
+                passes: 3,
+                unsafe: true,
+                ecma: 7


ecma 7 means ECMAScript 2017? or 2016?
anyhow we are officially targeting ECMAScript 2015, so this does not seem to make sense.

bd82 · 2021-11-03T20:19:55Z

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 😄

NaridaL · 2021-11-03T22:04:55Z

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.

bd82 · 2021-11-03T23:32:07Z

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:

ESM Bundle Improvements #1698

bd82 · 2021-11-04T02:28:49Z

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

chore(cjs/esm): Bundle module and use package exports node-fetch/node-fetch#1227

bd82 · 2021-11-04T02:48:23Z

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

Reducing maintenance TCO.
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 · 2022-08-13T12:19:01Z

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.

chore: use rollup instead of webpack and esbuild

ce4f39d

NaridaL force-pushed the rollup branch from 6da2a23 to ce4f39d Compare November 3, 2021 07:30

NaridaL mentioned this pull request Nov 3, 2021

ESM Bundle Improvements #1698

Closed

NaridaL added 2 commits November 3, 2021 17:43

fixes

a3c4472

fixes

63b2bb5

NaridaL commented Nov 3, 2021

View reviewed changes

NaridaL added 2 commits November 3, 2021 18:14

fixes

d63d065

fixes

6d397a6

bd82 reviewed Nov 3, 2021

View reviewed changes

packages/chevrotain/rollup.config.js

{

input: "src/api.ts",

output: [

["es", false, true],

Copy link

Member

bd82 Nov 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do these combination mean?

bd82 reviewed Nov 3, 2021

View reviewed changes

bd82 closed this Aug 13, 2022

bd82 deleted the rollup branch June 29, 2023 22:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: use rollup instead of webpack and esbuild #1701

chore: use rollup instead of webpack and esbuild #1701

NaridaL commented Nov 3, 2021

NaridaL commented Nov 3, 2021

NaridaL Nov 3, 2021

bd82 commented Nov 3, 2021 •

edited

bd82 Nov 3, 2021

bd82 Nov 3, 2021

bd82 Nov 3, 2021

bd82 Nov 3, 2021

bd82 Nov 3, 2021

bd82 commented Nov 3, 2021

NaridaL commented Nov 3, 2021

bd82 commented Nov 3, 2021 •

edited

bd82 commented Nov 4, 2021

bd82 commented Nov 4, 2021

bd82 commented Aug 13, 2022

chore: use rollup instead of webpack and esbuild #1701

chore: use rollup instead of webpack and esbuild #1701

Conversation

NaridaL commented Nov 3, 2021

NaridaL commented Nov 3, 2021

NaridaL Nov 3, 2021

Choose a reason for hiding this comment

bd82 commented Nov 3, 2021 • edited

Issues / Cons

Major: (not working correctly)

Minor (possible concerns)

Pros

bd82 Nov 3, 2021

Choose a reason for hiding this comment

bd82 Nov 3, 2021

Choose a reason for hiding this comment

bd82 Nov 3, 2021

Choose a reason for hiding this comment

bd82 Nov 3, 2021

Choose a reason for hiding this comment

bd82 Nov 3, 2021

Choose a reason for hiding this comment

bd82 commented Nov 3, 2021

Long Term

NaridaL commented Nov 3, 2021

bd82 commented Nov 3, 2021 • edited

bd82 commented Nov 4, 2021

bd82 commented Nov 4, 2021

bd82 commented Aug 13, 2022

bd82 commented Nov 3, 2021 •

edited

bd82 commented Nov 3, 2021 •

edited