chore(build): build the CLI module with esbuild #4187

alicewriteswrongs · 2023-03-21T20:34:22Z

This adds a support for building Stencil with esbuild. This comprises:

a new subdirectory scripts/esbuild which holds all the esbuild-related stuff
support for bundling the cli/ bundle with esbuild
new package.json scripts build.esbuild and build.esbuild.watch for doing a "hybrid" build
new CI workflows to build Stencil with esbuild and then run the existing unit and e2e test suites against the build artifact

Right now this new build / bundle setup exists parallel to the existing, rollup-based build setup. The idea is that we'll continue to use rollup for building and releasing, but we'll gradually increase the scope of the esbuild-based pipeline until it can build all of stencil. As noted above, this PR starts with just the cli/ module.

The esbuild-based pipeline is also set up to first call the existing rollup-based build. It then deletes the cli/ module built by rollup and replaces it with its own output.

Esbuild has a few notable differences from rollup which are relevant here:

It can directly bundle and transform TypeScript, so we can go from TypeScript to bundled JS in one step (rather than with our Rollup pipeline, where we run tsc and then run Rollup on the JS emitted by tsc). This decreases a bit of build complexity because for instance esbuild can read the paths configuration in tsconfig.json, so we no longer need something analogous to our alias plugin which normally brings those aliases into the Rollup build.
It's very fast! the cli/ module builds in about 13ms.
It also strips documentation comments from the code, I think because it does an AST based transform and discards comments as part of parsing (or doesn't make an effort to keep them around). This results in smaller bundle sizes. It does have support for retaining certain specially-formatted comments which is a good thing because we use a few magic comments for Vite support.
OOB it supports pretty much all of what we're currently doing in our Rollup-based build pipeline, so we should be able to gradually port everything over.

Once this is merged we can start using esbuild for local development and also start to port over other modules. I have a working build already for the compiler/ directory, getting that set up for local development will really speed things up!

Pull request checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been reviewed and added / updated if needed (for bug fixes / features)
Build (npm run build) was run locally and any changes were pushed
Unit tests (npm test) were run locally and passed
E2E Tests (npm run test.karma.prod) were run locally and passed
Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

What is the current behavior?

no esbuild, rollup slow

What is the new behavior?

yes esbuild, rollup still slow!

Does this introduce a breaking change?

Yes
No

Testing

I've built and packed this and then run it in several little example projects and in Framework with no problems.

github-actions · 2023-03-21T20:36:17Z

`--strictNullChecks` error report

Typechecking with --strictNullChecks resulted in 1399 errors on this branch.

That's the same number of errors on main, so at least we're not creating new ones!

reports and statistics

Our most error-prone files

Path	Error Count
src/dev-server/index.ts	37
src/mock-doc/serialize-node.ts	36
src/dev-server/server-process.ts	32
src/compiler/build/build-stats.ts	27
src/compiler/output-targets/dist-lazy/generate-lazy-module.ts	25
src/compiler/style/test/optimize-css.spec.ts	23
src/testing/puppeteer/puppeteer-element.ts	23
src/compiler/prerender/prerender-main.ts	22
src/runtime/vdom/vdom-render.ts	20
src/runtime/client-hydrate.ts	19
src/screenshot/connector-base.ts	19
src/compiler/config/test/validate-paths.spec.ts	16
src/dev-server/request-handler.ts	15
src/compiler/prerender/prerender-optimize.ts	14
src/compiler/sys/stencil-sys.ts	14
src/compiler/transpile/transpile-module.ts	14
src/runtime/vdom/vdom-annotations.ts	14
src/sys/node/node-sys.ts	14
src/compiler/build/build-finish.ts	13
src/compiler/prerender/prerender-queue.ts	13

Our most common errors

Typescript Error Code	Count
TS2345	424
TS2322	398
TS18048	310
TS18047	100
TS2722	38
TS2532	34
TS2531	23
TS2454	14
TS2352	13
TS2769	10
TS2790	10
TS2538	8
TS2344	5
TS2416	4
TS2493	3
TS18046	2
TS2684	1
TS2488	1
TS2430	1

Unused exports report

There are 12 unused exports on this PR. That's the same number of errors on main, so at least we're not creating new ones!

Unused exports

File	Line	Identifier
src/runtime/bootstrap-lazy.ts	21	setNonce
src/screenshot/screenshot-fs.ts	18	readScreenshotData
src/testing/testing-utils.ts	198	withSilentWarn
src/utils/index.ts	145	CUSTOM
src/compiler/app-core/app-data.ts	25	BUILD
src/compiler/app-core/app-data.ts	115	Env
src/compiler/app-core/app-data.ts	117	NAMESPACE
src/compiler/fs-watch/fs-watch-rebuild.ts	123	updateCacheFromRebuild
src/compiler/types/validate-primary-package-output-target.ts	62	satisfies
src/compiler/types/validate-primary-package-output-target.ts	62	Record
src/testing/puppeteer/puppeteer-declarations.ts	485	WaitForEventOptions
src/compiler/sys/fetch/write-fetch-success.ts	7	writeFetchSuccessSync

alicewriteswrongs

just noting a few things

scripts/esbuild/cli.ts

alicewriteswrongs · 2023-03-21T20:37:18Z

scripts/esbuild/cli.ts

+    main: cjsFilename,
+    module: esmFilename,
+    types: dtsFilename,
+  });


likewise, this is copied too:

stencil/scripts/bundles/cli.ts

Lines 55 to 61 in 8301b79

writePkgJson(opts, opts.output.cliDir, {

name: '@stencil/core/cli',

description: 'Stencil CLI.',

main: cjsFilename,

module: esmFilename,

types: dtsFilename,

});

alicewriteswrongs · 2023-03-21T20:38:31Z

scripts/esbuild/util.ts

+    '@stencil/core/compiler': './compiler/stencil.js',
+    '@stencil/core/dev-server': './dev-server/index.js',
+    '@stencil/core/mock-doc': './mock-doc/index.cjs',
+    '@stencil/core/internal/testing': './internal/testing/index.js',


set up aliases which will point to the entry points of other bundles. Note that in combination with getEsbuildExternalModules, below, these will be marked as external and thus imported or required instead of being bundled in

alicewriteswrongs · 2023-03-21T20:39:04Z

scripts/esbuild/util.ts

+ * Node modules which should be universally marked as external
+ *
+ */
+const externalNodeModules = [


the CLI module doesn't pull most of these in, but they are used by other bundles

Is this a list of all the things we mark as external during the rollup build? Just so I know what to double check this list against 😆

it may not be totally exhaustive - on my other branch this was the external modules I collated while getting the cli, compiler, testing, and mock-doc bundles building with Esbuild, so when we go to tackle the others maybe we'll need a few more!

Man, I really wish there was a way to automatically generate this list (and the list of module aliases above). Feels like something easy to overlook.

Did you come up with all these by trial and error (like, does the build fail if we're missing one of these), or did you have to scan the code for each bundle to make sure you had all of them?

I based this on the existing external lists used for the rollup builds (see here, here, and here for instance) and then I think possibly had to add one or two things that weren't already present - it is a bit of an annoying thing, if we accidentally forget to add something we do want to have externalized to the list then esbuild (and rollup too!) will happily just pull it into the bundle.

rwaskiewicz

I'm still getting accustomed to the ESBuild "world", but I don't wanna hold back a round of review on that account alone. I left a few different types of comments. Those that are marked secondary/tertiary aren't all that important ATM, just small comments here and there. The ones that aren't marked as secondary/tertiary are the more important ones here. LMK if you have any questions!

esbuild.js

package.json

esbuild.js

scripts/esbuild/cli.ts

scripts/esbuild/util.ts

rwaskiewicz · 2023-03-23T12:39:58Z

scripts/esbuild/util.ts

+    // node module redirection
+    chalk: 'ansi-colors',


I'm not sure I understand this comment/why we need this entry for cli. I see references to something similar in alias-plugin.ts:

const empty = new Set([ // we never use chalk, but many projects still pull it in 'chalk', ]);

Is this doing something to prevent 'chalk' usage?

this isn't cli-specific, but actually from testing:

stencil/scripts/bundles/testing.ts

Lines 91 to 93 in 9173759

if (importee === 'chalk') {

return require.resolve('ansi-colors');

}

possibly in a no-browser-compilation world we don't need to worry about some of these things anymore (I hope so!)

rwaskiewicz · 2023-03-23T12:46:25Z

scripts/esbuild/util.ts

+ * Node modules which should be universally marked as external
+ *
+ */
+const externalNodeModules = [


Is this a list of all the things we mark as external during the rollup build? Just so I know what to double check this list against 😆

scripts/esbuild/cli.ts

alicewriteswrongs · 2023-04-04T18:37:17Z

Alright so I've just gotten some CI stuff together so that we now have essentially a parallel CI to our normal one which instead starts with the esbuild-based build and then runs the e2e, unit tests, etc (everything except browserstack)

tanner-reits

Took a look at most of this. Skipped over the actions stuff for now, but can review that if it's in a good state (wasn't sure if it's still a wip)

scripts/esbuild/cli.ts

tanner-reits · 2023-04-04T19:16:20Z

scripts/esbuild/util.ts

+ * Node modules which should be universally marked as external
+ *
+ */
+const externalNodeModules = [


Man, I really wish there was a way to automatically generate this list (and the list of module aliases above). Feels like something easy to overlook.

Did you come up with all these by trial and error (like, does the build fail if we're missing one of these), or did you have to scan the code for each bundle to make sure you had all of them?

alicewriteswrongs · 2023-04-13T15:50:20Z

Just took a little time to circle back on this and get CI passing, including the test.dist command which validates that things treeshake properly!

tanner-reits

I remember feeling like this was in a decent state when we first looked through a long time ago. I think the most important aspects are:

Bundle size should definitely be investigated before we rely on this full-time. If we wanna merge just to have CI run against this in the meantime I think that's fine
Is there anything left to remove from the removal of in-browser compilation

scripts/esbuild/util.ts

scripts/esbuild/cli.ts

alicewriteswrongs · 2023-10-17T14:36:25Z

I realized the issue that was leading to the larger bundle size. In the rollup-based CLI bundle script we externalize the prompts library via the relativePathPlugin to redirect that import to sys/node/prompts.js:

stencil/scripts/bundles/cli.ts

Line 69 in b97dadc

relativePathPlugin('prompts', '../sys/node/prompts.js'),

Without that 'externalization' the prompts code and the code of all its dependencies are pulled in to the file, greatly expanding its size. After adding an alias to map the module name to that path the esbuild-bundled cli/index.js file is now substantially smaller than the rollup one! neat.

So if you build with rollup:

$ ls -al cli
total 1568
drwxr-xr-x   9 alice  staff   288B Oct 17 10:34 ./
drwxr-xr-x  52 alice  staff   1.6K Oct 17 10:20 ../
-rw-r--r--   1 alice  staff   7.4K Oct 17 10:34 config-flags.d.ts
-rw-r--r--   1 alice  staff    95K Oct 17 10:34 index.cjs
-rw-r--r--   1 alice  staff   284K Oct 17 10:34 index.cjs.map
-rw-r--r--   1 alice  staff   1.0K Oct 17 10:34 index.d.ts
-rw-r--r--   1 alice  staff    94K Oct 17 10:34 index.js
-rw-r--r--   1 alice  staff   284K Oct 17 10:34 index.js.map
-rw-r--r--   1 alice  staff   306B Oct 17 10:34 package.json

vs with esbuild:

$ ls -al cli
total 1328
drwxr-xr-x   9 alice  staff   288B Oct 17 10:35 ./
drwxr-xr-x  52 alice  staff   1.6K Oct 17 10:20 ../
-rw-r--r--   1 alice  staff   7.4K Oct 17 10:35 config-flags.d.ts
-rw-r--r--   1 alice  staff    56K Oct 17 10:35 index.cjs
-rw-r--r--   1 alice  staff   266K Oct 17 10:35 index.cjs.map
-rw-r--r--   1 alice  staff   1.0K Oct 17 10:35 index.d.ts
-rw-r--r--   1 alice  staff    54K Oct 17 10:35 index.js
-rw-r--r--   1 alice  staff   266K Oct 17 10:35 index.js.map
-rw-r--r--   1 alice  staff   306B Oct 17 10:35 package.json

Now I'm unsure why the esbuild one is so much smaller! What was in the ~40kb? I'll look into that a bit.

alicewriteswrongs · 2023-10-17T14:45:06Z

I think the difference in bundle size is basically down to the fact that the Rollup build retains all of our JSDoc comments while Esbuild removes them (the maintainer says this is a somewhat non-negotiable aspect of how esbuild works)

rwaskiewicz · 2023-10-17T14:50:41Z

@alicewriteswrongs Does that mean that /*!__STENCIL_STATIC_IMPORT_SWITCH__*/ also gets removed (a part of our support for Vite)?

alicewriteswrongs · 2023-10-17T15:20:09Z

I don't see a usage of that comment in the code that will be pulled into the cli module so I can't test this but I believe that can be retained because esbuild recognizes it as a "legal comment". It looks like we can configure the behavior we want, i.e. retaining it precisely where it is or the default behavior which is to move it to the end of the bundled file (I don't know exactly what that comment does so I'm not sure how we need to treat it).

alicewriteswrongs · 2023-10-17T15:27:39Z

Also just added an enhancement which is to add a way to watch the files built by esbuild, using a little helper function that takes in esbuild configs and, based on the BuildOptions, will either run them in watch mode or in synchronous mode

alicewriteswrongs

just noting a few things I just changed

alicewriteswrongs · 2023-10-18T14:25:17Z

scripts/esbuild/util.ts

+export function getBaseEsbuildOptions(): ESBuildOptions {
+  return {
+    bundle: true,
+    legalComments: 'inline',
+    logLevel: 'info',
+  };
+}


This sets up a few base options which I think we'll always want across all future esbuild-based bundles we add, I thought we should probably just canonize this right now (especially the legalComments bit based on @rwaskiewicz's comment)

alicewriteswrongs · 2023-10-18T14:28:35Z

scripts/esbuild/util.ts

+export function runBuilds(builds: ESBuildOptions[], opts: BuildOptions): Promise<(void | ESBuildResult)[]> {
+  if (opts.isWatch) {
+    return Promise.all(
+      builds.map(async (buildConfig) => {
+        const context = await esbuild.context(buildConfig);
+        return context.watch();
+      }),
+    );
+  } else {
+    return Promise.all(builds.map(esbuild.build));
+  }
+}


this lil helper will take an array of esbuild configs and our own Stencil-specific BuildOptions object and if the isWatch property is set on the BuildOptions it will run a watch-mode build using esbuild.context, otherwise it will just use esbuild.build to do a single build.

The idea is that each of our bundle functions would create a series of esbuild configurations, do any other setup that is necessary, and then return the result of calling this function with those build configs. Then in our main entry point we use Promise.all to await the result of all the builds.

also kind of funny that the esbuild interface is also called BuildOptions, so I've taken to aliasing it as ESBuildOptions

tanner-reits

This adds a support for building Stencil with [esbuild](https://esbuild.github.io/). This comprises: - a new subdirectory `scripts/esbuild` which holds all the esbuild-related stuff - support for bundling the `cli/` bundle with esbuild - new `package.json` scripts `build.esbuild` and `build.esbuild.watch` for doing a "hybrid" build - new CI workflows to build Stencil with esbuild and then run the existing unit and e2e test suites against the build artifact Right now this new build / bundle setup exists parallel to the existing, rollup-based build setup. The idea is that we'll continue to use rollup for building and releasing, but we'll gradually increase the scope of the esbuild-based pipeline until it can build all of stencil. As noted above, this PR starts with just the `cli/` module. The esbuild-based pipeline is also set up to first call the existing rollup-based build. It then deletes the `cli/` module built by rollup and replaces it with its own output. Esbuild has a few notable differences from rollup which are relevant here: - It can directly bundle and transform TypeScript, so we can go from TypeScript to bundled JS in one step (rather than with our Rollup pipeline, where we run `tsc` and then run Rollup on the JS emitted by `tsc`). This decreases a bit of build complexity because for instance esbuild can read the `paths` configuration in `tsconfig.json`, so we no longer need something analogous to our [alias plugin](https://github.com/ionic-team/stencil/blob/b97dadc967b1fde892cb75a544b1eecd2361b194/scripts/bundles/plugins/alias-plugin.ts) which normally brings those aliases into the Rollup build. - It's very fast! the `cli/` module builds in about 13ms. - It also strips documentation comments from the code, I think because it does an AST based transform and discards comments as part of parsing (or doesn't make an effort to keep them around). This results in smaller bundle sizes. It does have support for retaining [certain specially-formatted comments](https://esbuild.github.io/api/#legal-comments) which is a good thing because we use a few magic comments for Vite support. - OOB it supports pretty much all of what we're currently doing in our Rollup-based build pipeline, so we should be able to gradually port everything over. Once this is merged we can start using esbuild for local development and also start to port over other modules.

alicewriteswrongs requested a review from a team as a code owner March 21, 2023 20:34

github-actions bot requested review from rwaskiewicz and tanner-reits March 21, 2023 20:34

alicewriteswrongs commented Mar 21, 2023

View reviewed changes

alicewriteswrongs force-pushed the ap/esbuild-cli-bundle branch from e35f721 to 99546d4 Compare March 21, 2023 20:40

rwaskiewicz reviewed Mar 23, 2023

View reviewed changes

alicewriteswrongs force-pushed the ap/esbuild-cli-bundle branch 8 times, most recently from 2b4d653 to 85b80b0 Compare April 4, 2023 17:38

tanner-reits requested changes Apr 4, 2023

View reviewed changes

alicewriteswrongs requested review from rwaskiewicz and tanner-reits April 6, 2023 13:28

alicewriteswrongs force-pushed the ap/esbuild-cli-bundle branch from 336b563 to 43f6029 Compare April 13, 2023 13:53

alicewriteswrongs force-pushed the ap/esbuild-cli-bundle branch from 3fdee6b to e474e3b Compare April 26, 2023 13:40

alicewriteswrongs force-pushed the ap/esbuild-cli-bundle branch from e474e3b to 4a40c34 Compare June 5, 2023 15:23

alicewriteswrongs force-pushed the ap/esbuild-cli-bundle branch from 4a40c34 to 776a9af Compare June 28, 2023 15:11

alicewriteswrongs force-pushed the ap/esbuild-cli-bundle branch 2 times, most recently from ee26959 to a629d74 Compare July 31, 2023 19:01

alicewriteswrongs force-pushed the ap/esbuild-cli-bundle branch 2 times, most recently from 623cfc0 to e377789 Compare August 8, 2023 14:03

alicewriteswrongs force-pushed the ap/esbuild-cli-bundle branch from e377789 to 23cb0ad Compare September 28, 2023 18:51

alicewriteswrongs force-pushed the ap/esbuild-cli-bundle branch from 000328e to 2b5e571 Compare October 10, 2023 15:21

tanner-reits reviewed Oct 11, 2023

View reviewed changes

scripts/esbuild/util.ts Outdated Show resolved Hide resolved

scripts/esbuild/cli.ts Outdated Show resolved Hide resolved

scripts/esbuild/cli.ts Outdated Show resolved Hide resolved

alicewriteswrongs force-pushed the ap/esbuild-cli-bundle branch 2 times, most recently from 506fd39 to 995558f Compare October 17, 2023 14:29

alicewriteswrongs force-pushed the ap/esbuild-cli-bundle branch 6 times, most recently from 2d7a53e to 2df0a20 Compare October 17, 2023 20:29

alicewriteswrongs requested review from tanner-reits and rwaskiewicz October 17, 2023 20:59

alicewriteswrongs commented Oct 18, 2023

View reviewed changes

alicewriteswrongs changed the title ~~chore(build): build the CLI module with esbuild (WIP)~~ chore(build): build the CLI module with esbuild Oct 18, 2023

alicewriteswrongs force-pushed the ap/esbuild-cli-bundle branch from c9e592a to 1aced01 Compare October 18, 2023 18:08

rwaskiewicz approved these changes Oct 18, 2023

View reviewed changes

tanner-reits approved these changes Oct 18, 2023

View reviewed changes

alicewriteswrongs force-pushed the ap/esbuild-cli-bundle branch from 1aced01 to 9b3987e Compare October 18, 2023 23:32

alicewriteswrongs enabled auto-merge October 18, 2023 23:33

alicewriteswrongs added this pull request to the merge queue Oct 18, 2023

Merged via the queue into main with commit 036e63c Oct 18, 2023
96 checks passed

alicewriteswrongs deleted the ap/esbuild-cli-bundle branch October 18, 2023 23:54

alicewriteswrongs mentioned this pull request Nov 17, 2023

refactor(build): add esbuild-plugin-replace to CLI bundle #5083

Closed

2 tasks

alicewriteswrongs mentioned this pull request Apr 17, 2024

chore(release): switch release scripts to use ESbuild #5678

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(build): build the CLI module with esbuild #4187

chore(build): build the CLI module with esbuild #4187

alicewriteswrongs commented Mar 21, 2023 •

edited

Loading

github-actions bot commented Mar 21, 2023 •

edited

Loading

alicewriteswrongs left a comment

alicewriteswrongs Mar 21, 2023

alicewriteswrongs Mar 21, 2023

alicewriteswrongs Mar 21, 2023

rwaskiewicz Mar 23, 2023

alicewriteswrongs Mar 23, 2023

tanner-reits Apr 4, 2023

alicewriteswrongs Apr 4, 2023

rwaskiewicz left a comment

rwaskiewicz Mar 23, 2023

alicewriteswrongs Mar 23, 2023

alicewriteswrongs Mar 23, 2023

rwaskiewicz Mar 23, 2023

alicewriteswrongs commented Apr 4, 2023

tanner-reits left a comment

tanner-reits Apr 4, 2023

alicewriteswrongs commented Apr 13, 2023

tanner-reits left a comment

alicewriteswrongs commented Oct 17, 2023 •

edited

Loading

alicewriteswrongs commented Oct 17, 2023

rwaskiewicz commented Oct 17, 2023

alicewriteswrongs commented Oct 17, 2023

alicewriteswrongs commented Oct 17, 2023

alicewriteswrongs left a comment

alicewriteswrongs Oct 18, 2023

alicewriteswrongs Oct 18, 2023

tanner-reits left a comment

	writePkgJson(opts, opts.output.cliDir, {
	name: '@stencil/core/cli',
	description: 'Stencil CLI.',
	main: cjsFilename,
	module: esmFilename,
	types: dtsFilename,
	});

	if (importee === 'chalk') {
	return require.resolve('ansi-colors');
	}

chore(build): build the CLI module with esbuild #4187

chore(build): build the CLI module with esbuild #4187

Conversation

alicewriteswrongs commented Mar 21, 2023 • edited Loading

Pull request checklist

Pull request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Testing

github-actions bot commented Mar 21, 2023 • edited Loading

--strictNullChecks error report

reports and statistics

Unused exports report

alicewriteswrongs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwaskiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicewriteswrongs commented Apr 4, 2023

tanner-reits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicewriteswrongs commented Apr 13, 2023

tanner-reits left a comment

Choose a reason for hiding this comment

alicewriteswrongs commented Oct 17, 2023 • edited Loading

alicewriteswrongs commented Oct 17, 2023

rwaskiewicz commented Oct 17, 2023

alicewriteswrongs commented Oct 17, 2023

alicewriteswrongs commented Oct 17, 2023

alicewriteswrongs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanner-reits left a comment

Choose a reason for hiding this comment

alicewriteswrongs commented Mar 21, 2023 •

edited

Loading

github-actions bot commented Mar 21, 2023 •

edited

Loading

`--strictNullChecks` error report

alicewriteswrongs commented Oct 17, 2023 •

edited

Loading