Repository navigation
Webpack: opt-in filesystem cache + stop wiping .cache/ on clean - #49174
Conversation
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Code Coverage SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
anomiex
left a comment
There was a problem hiding this comment.
This seems like it may be worth pursuing for its own sake, not only as a follow-up to #49173.
But before changing this, it may be worth looking into why we're wiping the .cache dirs in the first place.
The babel cache at
.cache/babel/is unaffected.
It's completely affected, in that it's no longer being deleted on every build.
Unfortunately git archaeology doesn't turn up anything obvious. The first place adding it seems to be #21814, which has no explanation for why it's doing it; the original version of the PR also deleted vendor and node_modules too, so it may have just been an overzealous cleanup. Most of the other additions I see seem like they may well just be copy-pasting from that.
Lacking any obvious reason for the deletion, I decided to try a simple experiment: I did a build of the eight packages from trunk, and then a dev build of the eight packages with this PR, and then a production build of the eight packages from this PR. The two production builds were the same, so 🤷 it's probably not that, at least.
| @@ -96,6 +96,24 @@ const watchOptions = { | |||
| ignored: [ '**/node_modules', '**/dist', '**/vendor' ], | |||
| }; | |||
|
|
|||
| // Filesystem cache rooted at the consumer's project directory. Opt-in: consumers | |||
There was a problem hiding this comment.
Everything at the top level is opt-in. Usage instructions are in the readme. There's no point to having this gigantic comment here too.
There was a problem hiding this comment.
Removed — trimmed to a two-line note; usage lives only in the readme now.
| // To invalidate the cache after upgrading webpack-config or changing your own | ||
| // webpack config, delete `.cache/webpack/` in the project. (We deliberately don't | ||
| // list config files in `buildDependencies.config` from here — the consumer's config | ||
| // path isn't known to this module. Consumers can extend this object with their own | ||
| // `buildDependencies.config: [ require.resolve('./webpack.config.js') ]` if they | ||
| // want config-change invalidation.) |
There was a problem hiding this comment.
This seems rather unfortunate, and is probably part of the reason we delete the .cache dir in the first place.
You might do better by exporting a function rather than an object, and requiring the caller to pass in __filename / import.meta.filename so it can do things more correctly by default.
There was a problem hiding this comment.
Done. cache is now cache( configFile ); callers pass __filename, and it sets buildDependencies: { config: [ configFile ] } so the cache invalidates when the config changes.
| const cache = { | ||
| type: 'filesystem', | ||
| cacheDirectory: path.resolve( process.cwd(), '.cache/webpack' ), | ||
| name: `jetpack-${ mode }`, |
There was a problem hiding this comment.
It seems the default for name already includes the mode. We might do better leaving this unset.
There was a problem hiding this comment.
Dropped the explicit name (the hardcoded jetpack- was also wrong for a package shared beyond Jetpack).
One caveat surfaced while doing it: the three plugins/jetpack configs run as separate, concurrent webpack --config processes (see the watch / concurrently script) and none sets a top-level config name. With name unset they would all default to the same name and share — and clobber — a single pack under .cache/webpack. Since the helper now receives the config path, I namespace cacheDirectory per config file (.cache/webpack/<config basename>/) to keep them isolated. Verified a widget-visibility build lands in .cache/webpack/webpack.config.widget-visibility/default-development/, which also confirms the default name still namespaces by mode.
| Significance: patch | ||
| Type: changed | ||
|
|
||
| Build: stop wiping `.cache/` on `pnpm clean` so persistent build caches (babel, webpack) survive cleans. |
There was a problem hiding this comment.
End users of the package won't care about this. No point in putting it in the changelog.
There was a problem hiding this comment.
Converted these to a Comment: field instead of a user-facing entry, so the change is still recorded (and the changelog-presence check still passes) without adding a line to the published changelog. Applied to all eight packages.
| Significance: patch | ||
| Type: changed | ||
|
|
||
| Build: stop wiping `.cache/` on `pnpm clean` so persistent build caches (babel, webpack) survive cleans. |
| Significance: patch | ||
| Type: changed | ||
|
|
||
| Build: stop wiping `.cache/` on `pnpm clean` so persistent build caches (babel, webpack) survive cleans. |
| Significance: patch | ||
| Type: changed | ||
|
|
||
| Build: stop wiping `.cache/` on `pnpm clean` so persistent build caches (babel, webpack) survive cleans. |
| Significance: patch | ||
| Type: changed | ||
|
|
||
| Build: stop wiping `.cache/` on `pnpm clean` so persistent build caches (babel, webpack) survive cleans. |
| Significance: patch | ||
| Type: changed | ||
|
|
||
| Build: enable webpack's filesystem cache for the main, extensions, and widget-visibility bundles. Reduces local incremental rebuild time substantially after the first build; no effect on production output. |
There was a problem hiding this comment.
End users of the plugin really won't care about this.
There was a problem hiding this comment.
Same treatment — converted to a Comment: entry (and switched Type to other, since the plugin changelog vocabulary does not include changed).
|
Pushed a round addressing the review:
On the babel cache: you are right, my wording was backwards — it is very much affected, in that |
anomiex
left a comment
There was a problem hiding this comment.
Looking good! I left some suggestions on the documentation.
The eight package changelogs and the plugin changelog are now Comment:-only (no user-facing lines).
It seems instead they got deleted entirely?
b7d2da6 to
f0cc884
Compare
You seem to have lost the fixes from https://github.com/Automattic/jetpack/compare/b2b55290df7549bae7b76a3639eecc12f1da2e3b..b7d2da683c4b024aa4d2b5011f2c3e2a90fca6de |
974e96e to
bcc085c
Compare
anomiex
left a comment
There was a problem hiding this comment.
One suggestion inline. Other than that, looks reasonable, let's try it!
The jetpack plugin changelogger only allows major/enhancement/compat/bugfix/other; "changed" is not valid. Use "other" for this non-user-facing build change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…fig, skip in CI; drop user-facing changelog noise (cherry picked from commit 927946047b67476d6dc65a354d3c348968096171)
Co-authored-by: Brad Jorsch <anomiex@users.noreply.github.com> (cherry picked from commit b7d2da683c4b024aa4d2b5011f2c3e2a90fca6de)
…ache-export Co-authored-by: Brad Jorsch <anomiex@users.noreply.github.com>
cd57ff4 to
df9101e
Compare
Fixes # N/A
Proposed changes
Follow-up to #49173. Opt-in webpack filesystem cache + stops several
cleanscripts from wiping.cache/.The fast-build PR can detect "this project's JS sources changed since last build" via per-project SHA stamps, but the actual rebuild for those projects still pays full webpack cold-start cost (~60–90s on
plugins/jetpack's three concurrent bundles). Webpack's filesystem cache cuts incremental rebuilds to ~5–10s.This PR ships the foundation plus the highest-impact adoption:
@automattic/jetpack-webpack-confignow exports acache( configFile )helper. Consumers opt in by spreadingcache: jetpackWebpackConfig.cache( __filename )into their config. It returns{ type: 'filesystem', cacheDirectory: path.resolve( process.cwd(), '.cache/webpack', <configFile basename> ), store: 'pack', buildDependencies: { config: [ configFile ] } }:buildDependencies.config), and is namespaced per config file so a project's multiple — possibly concurrently built — configs don't share, and clobber, a single pack.nameis left unset: webpack's default already namespaces bymode, keeping production and development caches separate.undefinedwhenprocess.env.CIis set, since CI doesn't preserve the cache between builds.plugins/jetpackopts in across all three of its webpack configs (tools/webpack.config.js,webpack.config.extensions.js,webpack.config.widget-visibility.js) by passingcache: jetpackWebpackConfig.cache( __filename ). After the first cold build, subsequent rebuilds are cache-hit and dramatically faster.cleanscript audit: eight packages (blaze, classic-theme-helper, forms, masterbar, paypal-payments, podcast, search, yoast-promo) hadcleanscripts of the formrm -rf <build-output>/ .cache/. The.cache/wipe pre-dated this PR; per git archaeology it traces back to Search: Add essential scaffolding for package #21814 with no stated rationale (that PR also deletedvendor/node_modules, so it reads as an overzealous cleanup that was then copy-pasted around). Removed it from all eight so persistent build caches surviveclean. Build-output dirs (dist/,build/) are still wiped as before.Note this does change the babel cache's lifetime in those eight packages:
.cache/babel/was previously deleted on everyclean, and now persists. Per @anomiex's experiment on this PR, dev and production builds of the eight packages from this branch produced output identical to trunk, so persisting the cache appears safe.Other webpack-config consumers can opt in incrementally by passing
cache: jetpackWebpackConfig.cache( __filename )to their config — no required follow-up.Why opt-in rather than always-on
If we forced
cache: ...from inside webpack-config, every consumer would inherit it whether or not they're ready. Opt-in keeps each project's webpack config explicit about what it's doing, lets risky bundles (e.g. anything generating dynamic entry points) decide for themselves, and gives reviewers a single grep target.Other information
Related product discussion/links
Follows up on #49173 (the
jetpack fast-buildCLI command).Does this pull request change what data or activity we track or use?
No.
Testing instructions
Verify the cache export
Expected: an object with
type: 'filesystem',store: 'pack', acacheDirectoryending in.cache/webpack/webpack.config(namespaced by the config file's basename), andbuildDependencies.configcontaining the path you passed. Run it again withCI=trueset and it should printundefinedinstead.Verify the cache works end-to-end on
plugins/jetpackcd projects/plugins/jetpack && rm -rf .cache/webpack— start from a clean cache.time pnpm run build-widget-visibility(smallest bundle; orbuild-developmentfor all). Note duration (cold).[cached] … modules(locally: ~800ms → ~95ms for widget-visibility).ls .cache/webpack/shows a populated, per-config-namespaced cache, e.g..cache/webpack/webpack.config.widget-visibility/default-development/.Verify
cleanno longer wipes.cache/For any of the 8 packages (e.g.
projects/packages/forms):mkdir -p .cache/babel && touch .cache/babel/probe.pnpm run clean, then check.cache/babel/probestill exists. The build-output dirs (dist/,build/) should be gone.Verify no regression elsewhere
node --check projects/plugins/jetpack/tools/webpack.config.js— no errors (also for.extensions.jsand.widget-visibility.js).jetpack build plugins/jetpackshould still produce correct output. The cache only affects build time, not output.