-
Notifications
You must be signed in to change notification settings - Fork 795
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
chore(build): remove sourcemaps, minified compiler build #5645
Conversation
|
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 |
PR built and packed!Download the tarball here: https://github.com/ionic-team/stencil/actions/runs/8633844862/artifacts/1401993138 If your browser saves files to
|
4f955ac
to
d2864e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a search for stencil.min.js
across the entire Ionic Org in GitHub, and found a couple of places I think we should also look into as a part of this effort:
-
This section of the Stencil Site. I think that we can safely remove the offending sentence in the same files that Christian updated in ionic-team/stencil-site@999d831
-
We still have a reference to this file in the compiler itself, at
stencil/src/compiler/sys/stencil-sys.ts
Line 146 in b3886aa
return sys.getRemoteModuleUrl({ moduleId: '@stencil/core', path: 'compiler/stencil.min.js' }); createSystem
fn) - can you swap this out withstencil.js
and validate that works in a small Node project that callscreateSystem
?
Related to ionic-team/stencil#5645 STENCIL-1242
@rwaskiewicz good catch, opened a docs PR here: ionic-team/stencil-site#1397 |
2d1925f
to
e4799aa
Compare
e4799aa
to
9c56583
Compare
@alicewriteswrongs it looks like the second part of my original feedback wasn't addressed (#5645 (review))
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bah, sorry, meant to "request changes" rather than comment just now
Related to ionic-team/stencil#5645 STENCIL-1242
yeah was just looking at that, I made that change and then installed in a project locally and created a small node script to import and use the compiler API and it works fine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once the change to src/compiler/sys/stencil-sys.ts
is made
just pushed the commit that changes that lil function over |
This removes sourcemaps from the cli, compiler, and dev-server bundles and also removes the unused minified compiler build. All these changes are made to the esbuild-based build. We realized recently that the minified compiler is no longer necessary (it was previously used for in-browser compilation support, and was removed in #4317) and the sourcemaps are similarly not useful, but do take up a significant amount of space. Without this change a packed Stencil tarball is around 12MB, and with this change it drops to 3.5MB. This also adds a check to the Esbuild-based build pipeline where if you run it with the `DEBUG` environment variable it will build sourcemaps for all of the bundles built with Esbuild, so building like ```sh DEBUG=true npm run build ``` will produce a debugging-friendly local build with sourcemaps. STENCIL-1242
88b9b5b
to
0e3a303
Compare
…1397) Related to ionic-team/stencil#5645 STENCIL-1242
This removes sourcemaps from the cli, compiler, and dev-server bundles and also removes the unused minified compiler build. All these changes are made to the esbuild-based build.
We realized recently that the minified compiler is no longer necessary (it was previously used for in-browser compilation support, and was removed in #4317) and the sourcemaps are similarly not useful, but do take up a significant amount of space.
Without this change a packed Stencil tarball is around 12MB, and with this change it drops to 3.5MB.
STENCIL-1242
Documentation
Does this introduce a breaking change?
Testing
Test that you can build and use Stencil with this change.
Also try out the 'debug' build, which is building like so:
that should produce sourcemaps for all the files bundled with Esbuild.