-
-
Notifications
You must be signed in to change notification settings - Fork 112
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
change(web): common downcompile helpers 🧩 #8904
Merged
jahorton
merged 6 commits into
feature-esmodule-web-engine
from
change/web/common-downcompile-helpers
Jun 9, 2023
Merged
change(web): common downcompile helpers 🧩 #8904
jahorton
merged 6 commits into
feature-esmodule-web-engine
from
change/web/common-downcompile-helpers
Jun 9, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
mcdurdin
reviewed
Jun 1, 2023
Base automatically changed from
change/web/filesize-fixes
to
feature-esmodule-web-engine
June 8, 2023 05:59
d5975f5
to
e318c5c
Compare
Comparison: Before:
After:
That's over 22 KB in savings. The followup (#8964) should increase that by about another 8 KB. |
mcdurdin
approved these changes
Jun 8, 2023
This was referenced Jun 9, 2023
As #8964 (based on this) has passed all relevant build checks at this point, and both this one and that are approved, I'm moving ahead with merging now. (The Developer check is pending, admittedly, but that's it.) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Inspection of the results of our
esbuild
-bundled products indicates that it's pretty cautious with ES5 code... enough so that without a little help, this can result in somewhat significant filesize bloat for us.By default, TS directly emits down-compilation "helper" functions - essentially, polyfill helpers for ES6+ features - into ES5-targeting compilations. This was "fine" when everything was namespace-based - after all, we had
<reference path="..."/>
links that essentially compiled all of the relevant code, at once, into a single file. One destination file, one set of "downcompile helpers".With a little extrapolation, note that now, with modularized code, we instead have over a hundred individual module files. These, in turn, get bundled together for the final build. The problem... is that TS will, by default, directly emit those "downcompile helpers" into each and every one of those "over a hundred" module files that needs it. Only the ones that are needed by each specific file, granted, but it definitely adds up. The one that made me aware of this: the
__extends
helper (for class inheritance) gets duplicated at least 30 times - which represents several kilobytes of bloat by itself.Enter the TS compilation option: "importHelpers". The short of it: the developers of TypeScript also publish the library
tslib
- a centralized package of modules defining the same helpers. Using it tellstsc
to instead insert any neededimport { ... } from 'tslib';
statements into our compiled modules - with a single, centralized definition. This prevents "downcompile helper" duplication, allowing us to save on filesize.This does mean:
esbuild
/common/web/tslib
is added - it's the workaround that worked. Long story short... another problem arose due toes5
being "allowed", but not exactly first-class "supported", byesbuild
.esbuild
also provides thealias
feature, which was necessary to fully implement the workaround.Unfortunately, tree-shaking doesn't appear to be possible for
esbuild
+tslib
with the setup I could get working... at least not built-in. As a result, I believe it's better to leavetslib
out of the worker at present; it doesn't have nearly as much "downcompile helper" duplication as the rest of Web's codebase, so it may not win us anything there.That said, after further experimentation, I believe I've figured out how to use
esbuild
plugins to handletslib
treeshaking ourselves; this would save more KB and allow use oftslib
within the lm-worker. But that'll be a separate PR - #8964.The last notable change pattern - I'm getting a spurious error for downcompilations involving "array spreading":
[...array]
scenarios. Fortunately, our uses appear safe to convert to[].concat(array)
-style code, which poses no such issue.@keymanapp-test-bot skip