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): do not copy polyfills to the dist OT unless building es5 #5725

Merged
merged 1 commit into from
May 7, 2024

Conversation

alicewriteswrongs
Copy link
Contributor

@alicewriteswrongs alicewriteswrongs commented May 2, 2024

Prior to this change Stencil will copy polyfills to the dist output target whether or not the user has indicated they'll be necessary.

The polyfills comprise two things: copying the polyfills themselves into the 'loader' path, and adding code to the lazy-loader entry points which loads those polyfills. Instead of just assuming that the user wants this, we now gate this behavior on whether buildEs5 is set on the Stencil configuration.

fixes #5416
STENCIL-1288

What is the current behavior?

Right now we copy a bunch of polyfills which won't be necessary outside of an ES5-only environment even when buildEs5 is not set. This adds some extra bulk to the dist build output.

See #5416 for more details.

What is the new behavior?

If the user has set buildEs5 to true then we copy the polyfills, otherwise we don't do anything.

Documentation

Does this introduce a breaking change?

  • Yes
  • No

Testing

The best way to test this is to build a sample stencil project with @stencil/core@latest and then with a version based on this build and then compare the output.

First do:

cd /tmp
npm init stencil@latest component test-no-polyfills
cd test-no-polyfills
npm install
npm run build

then edit .gitignore to remove dist/ and loader/ from the ignore. Then you can follow the setup steps in CONTRIBUTING.md for local dev (e.g. the tsconfig paths stuff).

Then if you check out this branch in your stencil repo locally and run npm link you should be able to use this script to test things out in your new stencil project:

#!/bin/bash

npm unlink @stencil/core

npm install @stencil/core@latest

npm run build

git add .

git commit -m "build with @stencil/core@latest"

npm link @stencil/core

npm run build

git add .

git commit -m "build with dev build"

save that to like test.sh or something in your little project, throw a chmod +x on there and you're good to go.

Then you can do

git diff $(git rev-parse @~)

to see the diff between the @stencil/core@latest build and the dev build

Copy link
Contributor

github-actions bot commented May 2, 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. 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 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 May 2, 2024

PR built and packed!

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

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

unzip -d ~/Downloads ~/Downloads/stencil-core-4.18.0-dev.1715087629.688dc51.tgz.zip && npm install ~/Downloads/stencil-core-4.18.0-dev.1715087629.688dc51.tgz

@alicewriteswrongs alicewriteswrongs force-pushed the ap/build-es5-polyfills branch 3 times, most recently from 2387c8a to dddbfad Compare May 3, 2024 19:11
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.

noting two things


const expectedIndexOutput = `export * from '../esm/polyfills/index.js';
export * from '../esm-es5/loader.js';`;
expect(writeFileSpy).toHaveBeenCalledWith(resolve('/my-test-dir/loader/index.js'), expectedIndexOutput);
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 funny (the resolve call I mean) but I had to do this to get the tests passing on CI on Windows, you know, sometimes you gotta do what you gotta do

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 new spec file, we didn't previously have coverage for this file in particular

@alicewriteswrongs alicewriteswrongs marked this pull request as ready for review May 3, 2024 19:20
@alicewriteswrongs alicewriteswrongs requested a review from a team as a code owner May 3, 2024 19:20
Prior to this change Stencil will copy polyfills to the `dist` output
target whether or not the user has indicated they'll be necessary.

The polyfills comprise two things: copying the polyfills themselves into
the 'loader' path, and adding code to the lazy-loader entry points which
loads those polyfills. Instead of just assuming that the user wants
this, we now gate this behavior on whether `buildEs5` is set on the
Stencil configuration.

fixes #5416
STENCIL-1288
@alicewriteswrongs alicewriteswrongs added this pull request to the merge queue May 7, 2024
Merged via the queue into main with commit 945df46 May 7, 2024
129 checks passed
@alicewriteswrongs alicewriteswrongs deleted the ap/build-es5-polyfills branch May 7, 2024 13:26
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.

bug: stencil build command generates the folder dist\esm\polyfills\ folder even if buildEs5=false in config
3 participants