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.

Sign up for GitHub

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

Jump to bottom

build(ts): eliminate duplicate helper code #1085

Merged
merged 1 commit into from
Aug 14, 2023
Merged

build(ts): eliminate duplicate helper code #1085

merged 1 commit into from
Aug 14, 2023

Conversation

tthvo
Copy link
Member

@tthvo tthvo commented Aug 12, 2023 •
edited
Loading

Welcome to Cryostat! 👋

Before contributing, make sure you have:

  • Read the contributing guidelines
  • Linked a relevant issue which this PR resolves
  • Linked any other relevant issues, PR's, or documentation, if any
  • Resolved all conflicts, if any
  • Rebased your branch PR on top of the latest upstream main branch
  • Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
  • Signed all commits using a GPG signature

To recreate commits with GPG signature git fetch upstream && git rebase --force --gpg-sign upstream/main

Related to #1039 (production optimization started there)
Also closes #1072

Description of the change:

Use importHelpers to tell TS to import helper code (i.e. needed for backwards compatibility or downleveling) from tslib. This helps reducing the web asset size by eliminating duplicate code.

Motivation for the change:

For certain downleveling operations, TypeScript uses some helper code for operations like extending class, spreading arrays or objects, and async operations. By default, these helpers are inserted into files which use them. This can result in code duplication if the same helper is used in many different modules.

References: https://www.typescriptlang.org/tsconfig#importHelpers

Patternfly-seed: https://github.com/patternfly/patternfly-react-seed/blob/62e92b78a64c93f3fa8c059e7482aaa3192e681d/tsconfig.json#L24

@tthvo tthvo requested a review from maxcao13 August 12, 2023 04:34
@github-actions github-actions bot added the needs-triage Needs thorough attention from code reviewers label Aug 12, 2023
@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-1085-1c9474af03f6322382c19704e5d105f5d22a7f3a sh smoketest.sh

@tthvo tthvo added feat New feature or request and removed needs-triage Needs thorough attention from code reviewers labels Aug 12, 2023
@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-1085-1c9474af03f6322382c19704e5d105f5d22a7f3a sh smoketest.sh

@andrewazores
Copy link
Member

This didn't seem to do much for bundle size when I built it locally, but we may as well at least enable the optimization.

Before:

$ du -h dist
2.5M	dist/fonts
6.7M	dist/images
14M	dist

After:

$ du -h dist/
2.5M	dist/fonts
6.7M	dist/images
14M	dist/

I also compared the filesizes of each generated bundle file within dist/ and most bundles were the same size - in fact, the only apparent changes were that a couple of the bundles in the "after" set were very slightly larger...

@tthvo
Copy link
Member Author

tthvo commented Aug 14, 2023 •
edited
Loading

Ahh interesting...I am guessing because we are targeting es5 so there are not a lot of things to be downleveled and also I upgraded tslib version in this PR that might also added a bit of size?

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-1085-4ba3a70745066552175ace1f24cf82a749d19829 sh smoketest.sh

@tthvo
Copy link
Member Author

tthvo commented Aug 14, 2023 •
edited
Loading

Actually, I am noticing the app.bundle.js is a little smaller? The tslib seems to be larger.

On main:

-rw-r--r--.  1 thvo thvo  658495 Aug 14 13:19 app.ee50b55be9a13e6a.bundle.js
-rw-r--r--.  1 thvo thvo    8753 Aug 14 13:23 npm.tslib.ba03b9efcf84c89a.bundle.js

On this PR:

-rw-r--r--. 1 thvo thvo  611289 Aug 14 13:19 app.d1141d346aa92e0a.bundle.js
-rw-r--r--. 1 thvo thvo   10269 Aug 14 13:19 npm.tslib.62121a22669af2e9.bundle.js

@andrewazores andrewazores merged commit 671d895 into cryostatio:main Aug 14, 2023
19 checks passed
@tthvo tthvo deleted the ts-config branch August 14, 2023 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewazores andrewazores andrewazores approved these changes

@ebaron ebaron Awaiting requested review from ebaron

@maxcao13 maxcao13 Awaiting requested review from maxcao13

Assignees
No one assigned
Labels
build feat New feature or request safe-to-test
Projects
No open projects
2.4.0 release
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tthvo @andrewazores