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 JavaScript Webpack tree-shaking #9217

Merged
merged 1 commit into from
Sep 15, 2023
Merged

Fix JavaScript Webpack tree-shaking #9217

merged 1 commit into from
Sep 15, 2023

Conversation

aduth
Copy link
Member

@aduth aduth commented Sep 15, 2023

🛠 Summary of changes

Fixes an issue where Webpack tree-shaking (dead code elimination) was not working, inflating the size of JavaScript bundles by including JavaScript for design system components not in use.

This appears to have regressed as part of the revisions to conditionNames in #8655, though it's not entirely clear to me the relation between conditionNames and tree-shaking, aside from the net result that Webpack was choosing to resolve to CommonJS over ESM. Experimenting with ordering of conditionNames did not yield a desired outcome, but it does work correctly using the ... placeholder found in Webpack's internal source.

Performance Impact:

NODE_ENV=production yarn build && ls public/packs/js/application-*.js | head -n 1 | xargs brotli-size

Before: 17.3kb
After: 1.76kb
Diff: -15.54kb (-89.8%)

📜 Testing Plan

NODE_ENV=production yarn build

Observe that the application-[digest].js bundle in public/packs/js does not contain references to character-count.

Verify no regressions in expected behavior of #8655.

changelog: Internal, Performance, Reduce size of JavaScript bundles
@aduth aduth merged commit 0fcc3a7 into main Sep 15, 2023
@aduth aduth deleted the aduth-js-cherry-pick branch September 15, 2023 18:24
@allthesignals
Copy link
Contributor

I'm curious whether this ... is "public" behavior (can we rely on it during minor/patch bumps). The pattern seems documented in other Webpack features. 🤷

@aduth
Copy link
Member Author

aduth commented Sep 15, 2023

I'm curious whether this ... is "public" behavior (can we rely on it during minor/patch bumps). The pattern seems documented in other Webpack features. 🤷

Hmm, yeah, that's interesting re: the documentation in other options. I would hope that would mean it's at least intended to be supported, just not documented? I could also check if there's anything in their specs referencing it as something to not be regressed.

I'd hope in the worst case it'd be pretty obvious for us if it breaks if everything stops resolving all of a sudden during a Webpack build 😅

@allthesignals
Copy link
Contributor

allthesignals commented Sep 15, 2023

I'm curious whether this ... is "public" behavior (can we rely on it during minor/patch bumps). The pattern seems documented in other Webpack features. 🤷

Hmm, yeah, that's interesting re: the documentation in other options. I would hope that would mean it's at least intended to be supported, just not documented? I could also check if there's anything in their specs referencing it as something to not be regressed.

I'd hope in the worst case it'd be pretty obvious for us if it breaks if everything stops resolving all of a sudden during a Webpack build 😅

There seem to be some examples that do but I don't know whether they're asserted for!

FWIW I've also seen some libraries enforce bundle size checks to make sure tree-shaking still works which could be useful here.

@aduth
Copy link
Member Author

aduth commented Sep 15, 2023

FWIW I've also seen some libraries enforce bundle size checks to make sure tree-shaking still works which could be useful here.

I like that idea. As things stand, it's a little too easy for this to regress and go unnoticed.

aduth added a commit that referenced this pull request Oct 11, 2023
aduth added a commit that referenced this pull request Oct 11, 2023
aduth added a commit that referenced this pull request Oct 11, 2023
* Add lint check for reasonable asset bundle sizes

changelog: Internal, Automated Testing, Add test for reasonable asset bundle size

* TEMPORARY: Revert "Fix JavaScript dead code elimination (#9217)"

This reverts commit 0fcc3a7.

* Revert "TEMPORARY: Revert "Fix JavaScript dead code elimination (#9217)""

This reverts commit af166f2.
@aduth
Copy link
Member Author

aduth commented Oct 11, 2023

FWIW I've also seen some libraries enforce bundle size checks to make sure tree-shaking still works which could be useful here.

I like that idea. As things stand, it's a little too easy for this to regress and go unnoticed.

Done in #9353

jmdembe added a commit that referenced this pull request Oct 12, 2023
* LG-11082 Add Conditional Text To FullAddressSearch Component (#9331)

* Add conditional text to view

* add new tests

* Add period to display text

* package version increase from 3.1.0 to 3.1.1

* fix linter errors

* changelog: Upcoming feature, USPS Full Address Search, Added conditional logic to display/hide text on the Find a participating Post Office view that will display in Help Center only

* Integrate personal key feature specs into end_to_end_idv feature specs (#9336)

Since feature specs run slowly, it's better to check assertions as part of a single longer spec
rather than restart identity verification over and over. This removes several long-running feature
specs from the test suite.

[skip changelog]

* Update specs to initialize session as HashWithIndifferentAccess (#9347)

changelog: Internal, Automated Testing, Improve accuracy of session stubbing in tests

* Change `<b>` tags to `<strong>` for better accessibility and code consistency (#9349)

* Change `<b>` tags to `<strong>` for better accessibility and code consistency

changelog: User-facing Improvements, Accessibility, Use strong html tag instead of b for emphasis

* Enable RSpec/LeakyConstantDeclaration rubocop (#9348)

* Enable RSpec/LeakyConstantDeclaration rubocop

changelog: Internal, Source code, Enable RSpec rubocop

* Use let instead of defining new class

---------

Co-authored-by: Mitchell Henke <mitchell.henke@gsa.gov>

* Sync TypeScript-ESLint versions (#9352)

changelog: Internal, Dependencies, Update dependencies to their latest versions

* LG-10037: display warning banner on gpo welcome back page if number of gpo letter requests exceeded (#9303)

* display warning banner on gpo welcome back page if gpo letter requests are spammed

changelog: User-Facing Improvements, Identity Verification, display warning banner if user has sent max letter requests within a time window

* handle if user has no gpo confirmatio codes

* Update app/views/idv/by_mail/enter_code/index.html.erb

Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>

* tests for alert banner for spammed gpo requesets

* happy linting

* fix extra space in alert_spam_warning_html i18n

* happy linting

* lintfix i18n

* lint line too long

* js tag removal from alert gpo spam banner spec

* integrate warning alert banner for spammed gpo letter requests into existing tests

* refactor test for gpo spam warning banner

* happy linting

* create before action to remove test order dependency

* happy linting

* define  gpo_verification_enabled in review app

* define  gpo_verification_enabled in review app

---------

Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>

* Upgrade to Rails 7.1 (#9333)

* fix otp missing translations

* rails 7.1

changelog: Internal, Dependencies, Upgrade to Rails 7.1

* fix untranslated webauthn verification

* LG-10837: Add New Piv Cac Logging for login visited (#9294)

* changelog: Internal Fixes, Authentication LG-10837: Piv Cac Logging fixes

* changelog: Internal, Authentication, Add Login visited for pivcac/change logging names to be uniform

* uniform spec test

* fix naming convention for piv cac

* update rspec

* add previous name

* Add lint check for reasonable asset bundle sizes (#9353)

* Add lint check for reasonable asset bundle sizes

changelog: Internal, Automated Testing, Add test for reasonable asset bundle size

* TEMPORARY: Revert "Fix JavaScript dead code elimination (#9217)"

This reverts commit 0fcc3a7.

* Revert "TEMPORARY: Revert "Fix JavaScript dead code elimination (#9217)""

This reverts commit af166f2.

* Update changelog script to reflect non-security Dependabot usage (#9354)

changelog: Internal, Changelog, Update changelog script to reflect non-security Dependabot usage

* Revert "Upgrade to Rails 7.1 (#9333)" (#9356)

This reverts commit f9a0cd0.

* LG-10812 | Report on all-time user count (#9350)

changelog: Internal, Reporting, Monthly report includes all-time user count

* Reorganize combined invoice report for easier manual runs (#9358)

changelog: Internal, Reporting, Reorganize combined-invoice-supplement-report

* Exclude 'IRS Attempt API: Event metadata' events from log results (#9360)

changelog: Internal, Data Requests, Exclude 'IRS Attempt API: Event metadata' events from log results

* Remove Guardfile, guard dependencies (#9364)

changelog: Internal, Dependencies, Remove unused testing dependencies

* LG-11066 Do not redirect users at the phone step unless they are phone and address rate limited (#9345)

Users are being rate limited and encounting the phone error screen even if they can still verify by mail. This commit changes the rate limit logic to allow users to proceed to the phone step if they can still verify their phone or complete verification by mail.

A side-effect of this change is a bug is fixed where the following situation would exist:

1. A user proofed by mail after exhausting phone attempts
2. The user goes to GPO entry and chooses to cancel and start over
3. The user is redirected to the welcome step to start over
4. The welcome step before action observes the user is phone rate limited and sends the user to the phone errors controller
5. The phone errors controller has a before action to confirm the user has completed the phone errors step; the user has not since in this session so they are redirected to the welcome step
6. Steps 4 and 5 complete until there are too many redirects

[skip changelog]

Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov>

---------

Co-authored-by: gina-yamada <125507397+gina-yamada@users.noreply.github.com>
Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov>
Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Co-authored-by: Mitchell Henke <mitchell.henke@gsa.gov>
Co-authored-by: Amir Reavis-Bey <amir.reavis-bey@gsa.gov>
Co-authored-by: Malick Diarra <malick.diarra@gsa.gov>
Co-authored-by: Matt Wagner <mattwagner@navapbc.com>
Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>
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.

3 participants