chore(esbuild): rm inline graceful-fs in dev-server #5625

rwaskiewicz · 2024-04-05T17:35:20Z

What is the current behavior?

GitHub Issue Number: N/A

What is the new behavior?

update the building of the dev-server such that we no longer inline graceful-fs in the dev-server. this saves us approximately 10kb in total - 20kb get removed from dev-server/server-process.js, and 10kb are added to sys/node/index.ts.

this is accomplished by removing graceful-fs from the shared alias list, as it's only needed in sys and dev-server. we them mark the library as "external" so that esbuild resolves to the version held on to by the sys submodule

Documentation

N/A

Does this introduce a breaking change?

Yes
No

Testing

Size Testing

Our internal tool for testing bundle size can be used here by

Generating the output for main (8ee071bf3aae4b2240b50f7af433035c8bf8aa49)
Generating the output for this branch (6b45145fc026b2b9f9baef19a7e26be46c64283a)
Comparing the output

This can be done like so:

COMMIT_SHA=8ee071bf3aae4b2240b50f7af433035c8bf8aa49 npm run compare > main.txt &&
COMMIT_SHA=6b45145fc026b2b9f9baef19a7e26be46c64283a npm run compare > out.txt

Then comparing the output:

Where we see the results described above

Smoke Testing

I smoke tested HMR by:

Building this branch (npm run clean && npm ci && npm run build && npm pack)
Spinning up a Stencil component starter (cd /tmp && npm init stencil@latest component gfs-test)
Install the generated tarball from step 1 in the component starter
npm start in the component starter to spin up the dev server
Update component contents, css styles, verify the changes are served

Other information

Could we minimize this further? Perhaps! This felt like a "good enough" win for now/this effort though. LMK if you think there's additional things you'd like to see

github-actions · 2024-04-05T17:36:54Z

`--strictNullChecks` error report

Typechecking with --strictNullChecks resulted in 1137 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/runtime/client-hydrate.ts	20
src/testing/puppeteer/puppeteer-element.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/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/compiler/transpile/transpile-module.ts	12
src/mock-doc/test/attribute.spec.ts	12

Our most common errors

Typescript Error Code	Count
TS2322	361
TS2345	344
TS18048	204
TS18047	82
TS2722	37
TS2532	24
TS2531	21
TS2454	14
TS2790	11
TS2352	9
TS2769	8
TS2538	8
TS2416	7
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-04-05T17:37:03Z

PR built and packed!

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

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

unzip -d ~/Downloads ~/Downloads/stencil-core-4.15.0-dev.1712762564.e5cee5c.tgz.zip && npm install ~/Downloads/stencil-core-4.15.0-dev.1712762564.e5cee5c.tgz

alicewriteswrongs

I tried this out locally and I don't think we can make this exact change. In my testing I did a build locally and then checked out the dev-server/server-process.js file and noticed that there is now a require('graceful-fs') in there. If you're in e.g. the Stencil starter this will work fine, because graceful-fs is present (pulled in as a dependency of puppeteer and jest) but if you don't have those packages installed then the require will fail and starting the dev server will just hang like this:

Screen.Recording.2024-04-10.at.10.35.41.AM.mov

(I think some error messages from the server process aren't getting piped through correctly)

so I think instead of just externalizing we'll need to use the externalAlias plugin we have which externalizes an import and also aliases it at the same time - I think that should ensure that the graceful-fs code isn't bundled but also ensure that we can resolve the module when it doesn't happen to be installed

gh won't let me make a suggestions on the relevant lines, but here's how you could do that:

diff --git a/scripts/esbuild/dev-server.ts b/scripts/esbuild/dev-server.ts
index 7c2b00750..4329321b2 100644
--- a/scripts/esbuild/dev-server.ts
+++ b/scripts/esbuild/dev-server.ts
@@ -11,7 +11,7 @@ import { bundleExternal, sysNodeBundleCacheDir } from '../bundles/sys-node';
 import { getBanner } from '../utils/banner';
 import { type BuildOptions, createReplaceData } from '../utils/options';
 import { writePkgJson } from '../utils/write-pkg-json';
-import { getBaseEsbuildOptions, getEsbuildAliases, getFirstOutputFile, runBuilds } from './util';
+import { externalAlias, getBaseEsbuildOptions, getEsbuildAliases, getFirstOutputFile, runBuilds } from './util';
 
 const CONNECTOR_NAME = 'connector.html';
 
@@ -76,8 +76,6 @@ export async function buildDevServer(opts: BuildOptions) {
     './ws.js',
     // open-in-editor-api is externally bundled
     './open-in-editor-api',
-    // prevent graceful-fs from being inlined in the bundle, saving us ~20kb
-    'graceful-fs',
   ];
 
   const devServerAliases = getEsbuildAliases();
@@ -108,7 +106,12 @@ export async function buildDevServer(opts: BuildOptions) {
     format: 'cjs',
     platform: 'node',
     write: false,
-    plugins: [esm2CJSPlugin(), contentTypesPlugin(opts), replace(createReplaceData(opts))],
+    plugins: [
+      esm2CJSPlugin(),
+      contentTypesPlugin(opts),
+      replace(createReplaceData(opts)),
+      externalAlias('graceful-fs', '../sys/node/graceful-fs.js'),
+    ],
     banner: {
       js: getBanner(opts, `Stencil Dev Server Process`, true),
     },

it might be a good idea to review other bundles to make sure they're functional without graceful-fs being installed in a test project since it's also being removed from the thing we return in getEsbuildAliases

update the building of the dev-server such that we no longer inline graceful-fs in the dev-server. this saves us approximately 10kb in total - 20kb get removed from dev-server/server-process.js, and 10kb are added to `sys/node/index.ts`. this is accomplished by removing graceful-fs from the shared alias list, as it's only needed in `sys` and `dev-server`. we them mark the library as "external" so that esbuild resolves to the version held on to by the `sys` submodule STENCIL-1243

rwaskiewicz · 2024-04-10T15:28:53Z

@alicewriteswrongs great catch, thanks!

I added your suggestion in d5f12aa, tested the with/without puppeteer case, and verified this didn't mess with the size of the compiler in some other way:

alicewriteswrongs

lgtm! bundle size going ⬇️

This uses the `externalAlias` plugin to 'externalize' the `graceful-fs` package from our `sys/node/index.js` bundle while also aliasing it to the file that we ship as part of the Stencil package at `sys/node/graceful-fs.js`. We did a similar thing in #5625

This uses the `externalAlias` plugin to 'externalize' the `graceful-fs` package from our `sys/node/index.js` bundle while also aliasing it to the file that we ship as part of the Stencil package at `sys/node/graceful-fs.js`. We did a similar thing in #5625 STENCIL-1268

…5647) This uses the `externalAlias` plugin to 'externalize' the `graceful-fs` package from our `sys/node/index.js` bundle while also aliasing it to the file that we ship as part of the Stencil package at `sys/node/graceful-fs.js`. We did a similar thing in #5625 STENCIL-1268

rwaskiewicz marked this pull request as ready for review April 5, 2024 18:00

rwaskiewicz requested a review from a team as a code owner April 5, 2024 18:00

rwaskiewicz requested review from christian-bromann, alicewriteswrongs and tanner-reits April 5, 2024 18:00

rwaskiewicz force-pushed the rw/dev-server-rm-graceful-inline branch from 6b45145 to f0b7596 Compare April 10, 2024 12:57

alicewriteswrongs suggested changes Apr 10, 2024

View reviewed changes

rwaskiewicz added 2 commits April 10, 2024 11:21

review(ap): externalize graceful-fs

d5f12aa

rwaskiewicz force-pushed the rw/dev-server-rm-graceful-inline branch from 5d94926 to d5f12aa Compare April 10, 2024 15:22

rwaskiewicz requested a review from alicewriteswrongs April 10, 2024 15:28

tanner-reits approved these changes Apr 10, 2024

View reviewed changes

alicewriteswrongs approved these changes Apr 10, 2024

View reviewed changes

rwaskiewicz added this pull request to the merge queue Apr 10, 2024

Merged via the queue into main with commit ab5bc2b Apr 10, 2024
121 checks passed

rwaskiewicz deleted the rw/dev-server-rm-graceful-inline branch April 10, 2024 16:34

alicewriteswrongs mentioned this pull request Apr 10, 2024

chore(build): exclude graceful-fs from the sys/node/index.js bundle #5647

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(esbuild): rm inline graceful-fs in dev-server #5625

chore(esbuild): rm inline graceful-fs in dev-server #5625

rwaskiewicz commented Apr 5, 2024 •

edited

Loading

github-actions bot commented Apr 5, 2024 •

edited

Loading

github-actions bot commented Apr 5, 2024 •

edited

Loading

alicewriteswrongs left a comment •

edited

Loading

rwaskiewicz commented Apr 10, 2024

alicewriteswrongs left a comment

chore(esbuild): rm inline graceful-fs in dev-server #5625

chore(esbuild): rm inline graceful-fs in dev-server #5625

Conversation

rwaskiewicz commented Apr 5, 2024 • edited Loading

What is the current behavior?

What is the new behavior?

Documentation

Does this introduce a breaking change?

Testing

Size Testing

Smoke Testing

Other information

github-actions bot commented Apr 5, 2024 • edited Loading

--strictNullChecks error report

reports and statistics

Unused exports report

github-actions bot commented Apr 5, 2024 • edited Loading

PR built and packed!

alicewriteswrongs left a comment • edited Loading

Choose a reason for hiding this comment

rwaskiewicz commented Apr 10, 2024

alicewriteswrongs left a comment

Choose a reason for hiding this comment

rwaskiewicz commented Apr 5, 2024 •

edited

Loading

github-actions bot commented Apr 5, 2024 •

edited

Loading

`--strictNullChecks` error report

github-actions bot commented Apr 5, 2024 •

edited

Loading

alicewriteswrongs left a comment •

edited

Loading