Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(build): address dist bundle size issue #5705

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

alicewriteswrongs
Copy link
Contributor

@alicewriteswrongs alicewriteswrongs commented Apr 25, 2024

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

This one is a little involved to test because the full reproduction involves ionic framework and an ionic angular project.

So! We need to do some setup.

First, you should get Stencil linked up to ionic-framework. In the Stencil repo you can do:

npm link

Then in Framework do:

cd core
npm link @stencil/core
cd ..

then for testing purposes you'll need to apply this patch:

https://gist.github.com/alicewriteswrongs/1a1967bfe4171546f5393a91bcabee95

This adds a script for building the @ionic/core and @ionic/angular packages called ./build_angular.sh and also makes some changes to tsconfig.json which are necessary for the npm link business to work correctly.

Once you have that in place you should also clone this reproduction of the issue:

https://github.com/alicewriteswrongs/stencil-ionic-angular-repro

it's in the test-test-test subdirectory. If you go in there there is a script called install_dev_versions.sh which will install the packed @ionic/core and @ionic/angular packages into this test project.

OK! So if you get all of that set up then you're ready to work on this.

First, you should confirm the issue. You can use a script like this to run through the whole repro:

#!/bin/bash

# first build stencil
npm run build

# then build angular in Ionic Framework
cd ~/Code/ionic-framework/
./build_angular.sh

# then build in the repro
cd ~/Code/scratch-stencil/stencil-4.17-issue/test-test-test/
./install_dev_versions.sh
npm run build

Adjust as-needed to account for where you have stencil, framework, and my reproduction cloned.

So first run that script with main checked out in Stencil, and confirm that the build output for the angular project should a main.XXXXX.js file with a size of ~700kb.

Then check out this branch and confirm that the size drops down to under 400kb.

Copy link
Contributor

github-actions bot commented Apr 25, 2024

--strictNullChecks error report

Typechecking with --strictNullChecks resulted in 1138 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 205
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 15 unused exports on this PR. Unfortunately, it looks like that's an increase of 1 over main 😞.

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 245 NODE_TYPES
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

Copy link
Contributor

github-actions bot commented Apr 25, 2024

PR built and packed!

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

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

unzip -d ~/Downloads ~/Downloads/stencil-core-4.17.1-dev.1714155715.54efd62.tgz.zip && npm install ~/Downloads/stencil-core-4.17.1-dev.1714155715.54efd62.tgz

@alicewriteswrongs alicewriteswrongs force-pushed the ap/bigger-angular-bundles-issue branch 2 times, most recently from eaa8358 to 2a90d3d Compare April 25, 2024 19:06
@alicewriteswrongs alicewriteswrongs marked this pull request as ready for review April 25, 2024 19:06
@alicewriteswrongs alicewriteswrongs requested a review from a team as a code owner April 25, 2024 19:06
@rwaskiewicz
Copy link
Contributor

rwaskiewicz commented Apr 25, 2024

@alicewriteswrongs When I run build-angular.sh from the root of the Framework repo, I get:

[ ERROR ]  unhandledRejection: Error: Could not resolve '../../mock-doc/index.cjs/constants' from
           ../../stencil/internal/hydrate/index.js at error
           (/Users/ryan/workspaces/stencil/compiler/stencil.js:9734:27) at ModuleLoader.handleResolveId
           (/Users/ryan/workspaces/stencil/compiler/stencil.js:27260:20) at
           /Users/ryan/workspaces/stencil/compiler/stencil.js:27254:77

~~Is there anything obvious I might be doing wrong here? ~~ Probably PEBKAC

Copy link
Contributor Author

@alicewriteswrongs alicewriteswrongs left a comment

Choose a reason for hiding this comment

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

just noting something

test.sh Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a little test script - I'm going to leave it in for the review and delete before merge

Copy link
Member

@tanner-reits tanner-reits left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

Awesome work 🎉

Verified that this works by:

  • creating a new Ionic project via ionic start photo-gallery tabs --type=angular --capacitor selecting NgModules
  • building @ionic/core using Stencil build with this branch
  • run npm run build
  • bundle size for main.62054bba8aec0c16.js before: 671523, bundle size after: 367433

👍

@alicewriteswrongs alicewriteswrongs force-pushed the ap/bigger-angular-bundles-issue branch from 4b498a0 to ae30834 Compare April 26, 2024 18:19
This fixes an issue where when @ionic/angular was built using Stencil
and then used in an angular project it would produce much larger bundle
sizes than it had previously. This issue started appearing in Stencil
v4.17.x, after we switched to using esbuild for our production builds.

The issue occurred because of how bundles in `internal/` were depending
on code in `mock-doc/`. In particular, in `src/runtime/dom-extras.ts` we
had an import from `@stencil/core/mock-doc`. In our bundles this was
being correctly externalized, so that `internal/client/index.js` for
instance would import from `'../../mock-doc/index.cjs'`, but the problem
is that when an Angular project then tried to bundle this file later it
would not tree-shake the import properly and the whole contents of
mock-doc would be injected into the bundle, bloating it by 400kb or so.

This is because esbuild, which `ng build` uses under the hood, does not
support tree-shaking in commonjs modules. See here for details:

https://esbuild.github.io/api/#tree-shaking

The simplest fix for this is to add the `NODE_TYPES` enum to
`src/utils/constants.ts` so that the runtime does not need to have any
dependency on `mock-doc` at all.
@alicewriteswrongs alicewriteswrongs force-pushed the ap/bigger-angular-bundles-issue branch from ae30834 to 41a0bbe Compare April 26, 2024 18:21
@alicewriteswrongs alicewriteswrongs added this pull request to the merge queue Apr 26, 2024
Merged via the queue into main with commit 0a7becc Apr 26, 2024
129 checks passed
@alicewriteswrongs alicewriteswrongs deleted the ap/bigger-angular-bundles-issue branch April 26, 2024 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants