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

feat(web): downcompile helper treeshaking 🧩 #8964

Merged

Conversation

jahorton
Copy link
Contributor

@jahorton jahorton commented Jun 8, 2023

From #8904:

That said, after further experimentation, I believe I've figured out how to use esbuild plugins to handle tslib treeshaking ourselves...

esbuild isn't able to figure it out on its own, as we're having it bundle ES5 JS files that result from our TS compilation, so we have to help it along. We can utilize esbuild's plugin feature as an "in" to perform treeshaking of tslib's importable helper definitions.

I might have invested a bit much into it, given the limited savings this affords us, but I've aimed to make the treeshaker reasonably maintainable.

  • If we ever update tslib versions and new helpers get added, build warnings will be logged about potential, as-of-yet-unverified helpers that could be added to the treeshaking detection system.
    • I opted for this, rather than have the detector assume all pattern-conforming names in tslib.js are treeshakable.
    • Said assumption fortunately holds for now... but we aren't in the position to enforce it, as we're not its maintainers.
  • While not perfect, the plugin tries to only treeshake function definitions that it can confidently handle. It'll abort treeshaking for a helper and report warnings if a pattern doesn't match expectations.
  • It will also abort and report build errors if something goes seriously wrong... like trying to 'treeshake' past the end of a file.

These reports are integrated within esbuild's output logs by use of the proper plugin return types & message forwarding. It'll even highlight the helper and its location within the main tslib.js file that's presenting a problem within the logged message, which gets hyperlinked in build logs (when viewed within VS Code's terminal, at least).

This saves us maybe another 8.5 KB in total after #8904; it's not that much, but it is something, at least. (That is over 33% of the increase noted by the check/web/filesize check reported for #8904.) And this optimization work was absolutely necessary for tslib to save us anything at all within the lm-worker due to its comparatively minimal use of downcompile-helpers.

@keymanapp-test-bot skip

@jahorton jahorton added this to the A17S14 milestone Jun 8, 2023
@jahorton jahorton requested a review from mcdurdin as a code owner June 8, 2023 08:04
@jahorton
Copy link
Contributor Author

jahorton commented Jun 8, 2023

Report from my local build with the current state of this PR:

❌ Oh dear! keymanweb.js is 14,198 bytes (3%) larger than 17.0.120, now 407,223 bytes

Compared against #8904:

❌ Oh dear! keymanweb.js is 23041 bytes (5%) larger than 17.0.120, now 416066 bytes

Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

LGTM. I wonder if this is something we could submit upstream to esbuild or typescript/tslib maintainers? It may be that they would be willing to take this on since it is a nice perf optimization for many common build chains

@jahorton
Copy link
Contributor Author

jahorton commented Jun 9, 2023

LGTM. I wonder if this is something we could submit upstream to esbuild or typescript/tslib maintainers? It may be that they would be willing to take this on since it is a nice perf optimization for many common build chains

I've linked this and its predecessor to the most relevant esbuild issue I could find: evanw/esbuild#2296.

For tslib, I've posted something similar at microsoft/tslib#173. They seem to be generally resistant to changing the importable scripts and their setup due to reasons documented at microsoft/tslib#180, so I'm hesistant to try contributing a 'fix' - as I'd likely break other patterns that they're concerned about.

Base automatically changed from change/web/common-downcompile-helpers to feature-esmodule-web-engine June 9, 2023 03:33
@jahorton jahorton merged commit 833f3ae into feature-esmodule-web-engine Jun 9, 2023
@jahorton jahorton deleted the feat/web/downcompile-helper-treeshaking branch June 9, 2023 03:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants