chore(scripts): swap from rollup to esbuild for main build #5420

alicewriteswrongs · 2024-02-28T19:22:14Z

This swaps over from Rollup to Esbuild for the main npm run build script in package.json. The existing Rollup based script is preserved as build.rollup.

This change causes our main CI run (.github/workflows/main.yml) to now use the Esbuild-based build, so the existing, supplemental CI run which previously was based on the Esbuild-based build pipeline now uses the 'legacy' (or nearly so) Rollup-based version. This requires a bit of renaming and so on.

STENCIL-1016

What is the current behavior?

GitHub Issue Number: N/A

What is the new behavior?

Documentation

Does this introduce a breaking change?

Yes
No

Testing

To test this out in the context of our GH-based releases I ran the dev and nightly releases against this branch:

dev release: https://github.com/ionic-team/stencil/actions/runs/8087583103
nightly release: https://github.com/ionic-team/stencil/actions/runs/8087594210

Both builds finished without error. The versions look crazily similar:

dev: 4.12.4-dev.1709157324.4664b01
nightly: 4.12.4-dev.1709157393.4664b01

github-actions · 2024-02-28T19:23:42Z

`--strictNullChecks` error report

Typechecking with --strictNullChecks resulted in 1139 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/dev-server/server-process.ts	32
src/compiler/prerender/prerender-main.ts	22
src/testing/puppeteer/puppeteer-element.ts	22
src/runtime/client-hydrate.ts	20
src/screenshot/connector-base.ts	19
src/runtime/vdom/vdom-render.ts	17
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/sys/node/node-sys.ts	14
src/compiler/prerender/prerender-queue.ts	13
src/compiler/sys/in-memory-fs.ts	13
src/runtime/connected-callback.ts	13
src/runtime/set-value.ts	13
src/compiler/output-targets/output-www.ts	12
src/compiler/transformers/test/parse-vdom.spec.ts	12
src/compiler/transformers/transform-utils.ts	12
src/mock-doc/test/attribute.spec.ts	12

Our most common errors

Typescript Error Code	Count
TS2322	361
TS2345	349
TS18048	201
TS18047	82
TS2722	37
TS2532	24
TS2531	21
TS2454	14
TS2790	11
TS2352	10
TS2769	8
TS2538	8
TS2416	6
TS2493	3
TS18046	2
TS2684	1
TS2430	1

Unused exports report

There are 14 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/utils/index.ts	269	normalize
src/utils/index.ts	7	escapeRegExpSpecialCharacters
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	61	satisfies
src/compiler/types/validate-primary-package-output-target.ts	61	Record
src/testing/puppeteer/puppeteer-declarations.ts	485	WaitForEventOptions
src/compiler/sys/fetch/write-fetch-success.ts	7	writeFetchSuccessSync

github-actions · 2024-02-28T19:23:56Z

PR built and packed!

Download the tarball here: https://github.com/ionic-team/stencil/actions/runs/8100518507/artifacts/1286871972

If your browser saves files to ~/Downloads you can install it like so:

unzip -d ~/Downloads ~/Downloads/stencil-core-4.12.4-dev.1709229735.7b1b682.tgz.zip && npm install ~/Downloads/stencil-core-4.12.4-dev.1709229735.7b1b682.tgz

alicewriteswrongs · 2024-02-28T19:26:06Z

One question I have with this PR for the team: does it make sense for us to keep the nightly and dev releases on Rollup, or to also swap them over to Esbuild?

Right now they are using Esbuild since they use the build.yml workflow, which previously called Rollup but now calls Esbuild.

rwaskiewicz

Changes LGTM - I think everything here should work; Prod releases still appear to use rollup (as we intended) and this shouldn't require any changes to our status checks in CI, nice!

Only ask here is that we manually kick off a dev and nightly build based on this branch just to kick the tires/make sure everything works as expected. Can you run both workflows and post the results/runs in the PR summary?

alicewriteswrongs · 2024-02-28T21:54:57Z

yeah good idea, PR summary updated with links to the gh actions runs for those

tanner-reits

One question, non-blocking if not needed

tanner-reits · 2024-02-29T16:30:27Z

package.json

-    "build": "npm run tsc.scripts && npm run tsc.prod && npm run rollup.prod.ci",
-    "build.esbuild": "npm run clean && npm run tsc.scripts && npm run tsc.prod && node scripts/build/esbuild/build.js --prod --ci",
-    "build.esbuild.watch": "npm run build.esbuild -- --watch",
+    "build.rollup": "npm run tsc.scripts && npm run tsc.prod && npm run rollup.prod.ci",


Do we need npm run clean at the start of this?

I don't think so, since the Rollup build clears out the directories here:

stencil/scripts/build.ts

Lines 44 to 52 in 1eec9a8

await Promise.all([

emptyDir(opts.output.cliDir),

emptyDir(opts.output.compilerDir),

emptyDir(opts.output.devServerDir),

emptyDir(opts.output.internalDir),

emptyDir(opts.output.mockDocDir),

emptyDir(opts.output.sysNodeDir),

emptyDir(opts.output.testingDir),

]);

Ah, but does it clear out the output from npm run tsc.scripts?

it does not - went ahead and just added it because why not

although I'll point out this is our current build script, so we're currently not clearing out scripts/ stuff between builds

although 🤔 maybe that's intentional so that the caching business that happens in there sticks around?

Yeah, exactly - I typically run it (clean) whenever I switch branches locally (npm run clean && npm ci && npm run build), but it does slow down some overall build time

yeah, ok I'm going to go ahead and leave the command as it is on main for now

This swaps over from Rollup to Esbuild for the main `npm run build` script in `package.json`. The existing Rollup based script is preserved as `build.rollup`. This change causes our main CI run (`.github/workflows/main.yml`) to now use the Esbuild-based build, so the existing, supplemental CI run which previously was based on the Esbuild-based build pipeline now uses the 'legacy' (or nearly so) Rollup-based version. This requires a bit of renaming and so on. STENCIL-1016

alicewriteswrongs marked this pull request as ready for review February 28, 2024 20:17

alicewriteswrongs requested a review from a team as a code owner February 28, 2024 20:17

alicewriteswrongs requested review from rwaskiewicz and tanner-reits February 28, 2024 20:17

rwaskiewicz suggested changes Feb 28, 2024

View reviewed changes

rwaskiewicz approved these changes Feb 28, 2024

View reviewed changes

tanner-reits approved these changes Feb 29, 2024

View reviewed changes

alicewriteswrongs force-pushed the ap/esbuild-rollup-switchover branch from 4664b01 to bee40b9 Compare February 29, 2024 17:12

alicewriteswrongs force-pushed the ap/esbuild-rollup-switchover branch from be24241 to 5f2237a Compare February 29, 2024 18:01

alicewriteswrongs enabled auto-merge February 29, 2024 18:01

alicewriteswrongs added this pull request to the merge queue Feb 29, 2024

Merged via the queue into main with commit 24623d7 Feb 29, 2024
121 checks passed

alicewriteswrongs deleted the ap/esbuild-rollup-switchover branch February 29, 2024 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(scripts): swap from rollup to esbuild for main build #5420

chore(scripts): swap from rollup to esbuild for main build #5420

alicewriteswrongs commented Feb 28, 2024 •

edited

Loading

github-actions bot commented Feb 28, 2024

github-actions bot commented Feb 28, 2024 •

edited

Loading

alicewriteswrongs commented Feb 28, 2024

rwaskiewicz left a comment

alicewriteswrongs commented Feb 28, 2024

tanner-reits left a comment

tanner-reits Feb 29, 2024

alicewriteswrongs Feb 29, 2024

rwaskiewicz Feb 29, 2024

alicewriteswrongs Feb 29, 2024

rwaskiewicz Feb 29, 2024

alicewriteswrongs Feb 29, 2024

	await Promise.all([
	emptyDir(opts.output.cliDir),
	emptyDir(opts.output.compilerDir),
	emptyDir(opts.output.devServerDir),
	emptyDir(opts.output.internalDir),
	emptyDir(opts.output.mockDocDir),
	emptyDir(opts.output.sysNodeDir),
	emptyDir(opts.output.testingDir),
	]);

chore(scripts): swap from rollup to esbuild for main build #5420

chore(scripts): swap from rollup to esbuild for main build #5420

Conversation

alicewriteswrongs commented Feb 28, 2024 • edited Loading

What is the current behavior?

What is the new behavior?

Documentation

Does this introduce a breaking change?

Testing

github-actions bot commented Feb 28, 2024

--strictNullChecks error report

reports and statistics

Unused exports report

github-actions bot commented Feb 28, 2024 • edited Loading

PR built and packed!

alicewriteswrongs commented Feb 28, 2024

rwaskiewicz left a comment

Choose a reason for hiding this comment

alicewriteswrongs commented Feb 28, 2024

tanner-reits left a comment

Choose a reason for hiding this comment

tanner-reits Feb 29, 2024

Choose a reason for hiding this comment

alicewriteswrongs Feb 29, 2024

Choose a reason for hiding this comment

rwaskiewicz Feb 29, 2024

Choose a reason for hiding this comment

alicewriteswrongs Feb 29, 2024

Choose a reason for hiding this comment

rwaskiewicz Feb 29, 2024

Choose a reason for hiding this comment

alicewriteswrongs Feb 29, 2024

Choose a reason for hiding this comment

alicewriteswrongs commented Feb 28, 2024 •

edited

Loading

`--strictNullChecks` error report

github-actions bot commented Feb 28, 2024 •

edited

Loading