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

[PM-18503] Webpack caching, minimize only on prod for Web #13508

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mzieniukbw
Copy link
Contributor

@mzieniukbw mzieniukbw commented Feb 21, 2025

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-18503

📔 Objective

Speeds up Web build for development.

Script: build:bit:dev:watch

  • On M3 Pro with NODE_OPTIONS="--max-old-space-size=8192"
  • Cached is any subsequent run of the same script with no or minimal changes to source code.
  • Measured using /usr/bin/time -l npm run build:bit:watch
Script Before After
No cache 65 seconds, 5.9GB max memory (The same)
Cached, minimal or no changes to source code (the same as Cold) 20-25s, 4.3GB max memory
Cached, a lot of changes to source code (the same as Cold) 42-45s, 5GB max memory

Conclusion: Any subsequent (cached) run is up to 45 seconds (65%) faster and requires up to 1.5GB less memory.

Did not observe any difference in our GH Actions pipeline.

📸 Screenshots

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

Copy link

codecov bot commented Feb 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 35.60%. Comparing base (077e0f8) to head (6879270).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #13508      +/-   ##
==========================================
- Coverage   35.60%   35.60%   -0.01%     
==========================================
  Files        3140     3141       +1     
  Lines       93042    93079      +37     
  Branches    16918    16925       +7     
==========================================
+ Hits        33132    33142      +10     
- Misses      57371    57400      +29     
+ Partials     2539     2537       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

github-actions bot commented Feb 21, 2025

Logo
Checkmarx One – Scan Summary & Details9e0353a6-f879-4b6d-89bb-da12895d67b3

Great job, no security vulnerabilities found in this Pull Request

@mzieniukbw mzieniukbw marked this pull request as ready for review February 21, 2025 14:30
@Hinton
Copy link
Member

Hinton commented Feb 21, 2025

You're not seeing an effect in CI because it's not persisted between runs.

Caching to filesystem in webpack is potentially dangerous, see https://github.com/webpack/changelog-v5/blob/master/guides/persistent-caching.md#opt-in. Some things I've observed with the existing memory cache is linked packages such as the SDK not being updated without restarting the devserver. This might now propagate into needing the user to manually delete the cache.

TLDR: Be careful as this might break assumptions people have around how builds works, and advanced workflows like npm link https://contributing.bitwarden.com/getting-started/sdk/internal/#web-clients.

Copy link
Contributor

@tangowithfoxtrot tangowithfoxtrot left a comment

Choose a reason for hiding this comment

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

Initial clean build took ~90 seconds on my M2 Mac. Subsequent cached builds took ~30 seconds.

Thanks for the very welcome speed-up!

@mzieniukbw
Copy link
Contributor Author

@Hinton i was afraid something might mess up with GH Actions, so i have not added this cache to any workflows, too risky.

I will check how this behaves with:

  • Changes in SDK
  • Changes in dependencies followed by npm ci

@mzieniukbw
Copy link
Contributor Author

I have adjusted the webpack caching configuration to pick up changes in:

  • symbolic linked local @bitwarden/sdk-internal dependency
  • npm ci invalidates cache by removing the stored cached data from root node_modules

I have tested above scenarios, including re-compiling sdk-internal with changes and it seems to be working now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants