[No QA] Compress web bundles with Brotli - #101730
roryabraham merged 8 commits into
Conversation
|
|
collectioneur
changed the title
|
@codex review |
dariusz-biela
left a comment
dariusz-biela
left a comment
There was a problem hiding this comment.
Looks good to me!
Note
One reminder for the follow-up CDN rewrite: Storybook assets under /docs/ don’t get .br copies, so exclude that path from the rewrite unless we also add compression for the Storybook build. This doesn’t block this PR.
|
@marufsharifi Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
| import {RsdoctorRspackPlugin} from '@rsdoctor/rspack-plugin'; | ||
| import {rspack} from '@rspack/core'; | ||
| import {execSync} from 'child_process'; | ||
| import CompressionPlugin from 'compression-webpack-plugin'; |
There was a problem hiding this comment.
This plugin was deprecated in favour of a new one a few days ago. Should we change? https://github.com/webpack/compression-webpack-plugin
There was a problem hiding this comment.
Let's use https://github.com/ramon-villain/compression-rspack-plugin if we can? Just want to do what we can to keep build times from creeping back up.
There was a problem hiding this comment.
Yes, the plugin was deprecated on September 19th. For now, it's just a README update (npm doesn't throw a warning and rspack still lists it as compatible). As for minimizer-webpack-plugin 5.11.0, it currently crashes under rspack. I’ve added a bit more context about this in the "Dependency note" section of the description.
Regarding compression-rspack-plugin: I'm a bit hesitant to introduce a dependency that only has 2 stars and hasn't seen any commits since March. And also it wouldn't improve our build times. It parallelizes across assets and compresses each asset with a single-threaded BrotliCompress. This is essentially doing the same thing as Node's zlib. Since our compression phase is bottlenecked by the largest single files, any per-asset parallelization will still hit the same ~12s floor. Their 3x speedup benchmark is based on 1,400 small assets, which doesn't reflect our use case.
If we'd prefer to avoid shipping a deprecated dependency, I think we have two good options:
- Keep it for now, open an issue/PR in rspack repo to fix the
minimizer-webpack-plugincompatibility, and migrate once it's resolved. - Or write our own custom compressor. It would only take about 40 lines of code (
processAssets+zlib.brotliCompress) with zero external dependencies, meaning we’d have full control over any future fixes.
I'd lean towards option 2 if we decide we definitely need to drop the current plugin. Which of these sounds better to you?
There was a problem hiding this comment.
Agree option 2 sounds better.
| ...(isDevelopment | ||
| ? [] | ||
| : [ | ||
| new CompressionPlugin<BrotliOptions>({ |
There was a problem hiding this comment.
I believe you need the filename parameter here.
The default according to the docs is [path][base].gz but we want .br extensions now.
There was a problem hiding this comment.
When algorithm is brotliCompress, the plugin defaults filename to [path][base].br (source). But for visibility, I’ll add this as an explicit value, thanks! 🙂
514b7d9 to
0bab458
Compare
|
I created our custom plugin and compared the generated brotli files against the previous version byte by byte. Everything works exactly as before 👌 |
| ...(process.env.ANALYZE_BUNDLE === 'true' ? [new RsdoctorRspackPlugin()] : []), | ||
| // Writes a Brotli 11 twin (`foo.js` -> `foo.js.br`) beside every deployable text/bytecode asset, so the CDN | ||
| // can serve it instead of compressing with gzip on the fly: 25-30% fewer bytes over the wire. | ||
| ...(isDevelopment ? [] : [new BrotliCompressionPlugin({test: /\.(?:js|css|html|svg|wasm|ttf)$/})]), |
There was a problem hiding this comment.
adhoc is not considered development, but you are discarding the files here https://github.com/Expensify/App/pull/101730/changes#diff-017793d1d9e32e06aac44f03cafe131439f7974fd77eb715949317045f9b6df3R139
There was a problem hiding this comment.
Good catch, thanks! All fixed
roryabraham
left a comment
roryabraham
left a comment
There was a problem hiding this comment.
plugin code LGTM 👍🏼
LGTM except for @justinpersaud's comment
marufsharifi
left a comment
marufsharifi
left a comment
There was a problem hiding this comment.
Just one minor suggestion. LGTM
| - name: Deploy to S3 | ||
| run: | | ||
| aws s3 cp --recursive --acl public-read "$GITHUB_WORKSPACE"/dist ${{ env.S3_BUCKET }}/ | ||
| aws s3 cp --recursive --acl public-read --exclude "*.br" "$GITHUB_WORKSPACE"/dist ${{ env.S3_BUCKET }}/ |
There was a problem hiding this comment.
NAB: /docs/* can silently return the NewDot app shell.
The dist here combines the web build (which has .br twins) with the Storybook build under ./dist/docs (which does not).
Because the CDN rewrite currently applies to /docs/*, a request like:
/docs/sb-manager/runtime.js → /docs/sb-manager/runtime.js.br
can miss the file and still return 200 HTML because of the SPA fallback. This can cause ERR_CONTENT_DECODING_FAILED or the browser to reject the script due to the text/html MIME type. A 404-based check won't catch it.
Could we please:
- Exclude
/docs/*from the CloudFront rewrite and add a comment explaining why. - Update the comment in
BrotliCompressionPlugin.ts:17— a missing twin does not necessarily mean a 404 because of the SPA fallback.
There was a problem hiding this comment.
Exclude /docs/* from the CloudFront rewrite and add a comment explaining why.
cc: @justinpersaud
I think we need this since /docs is the Storybook build, which doesn't go through rspack, so none of its assets have .br twins, but they land in the same bucket behind the same distribution. WDYT?
|
🚧 roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.4.96-0 🚀
|
7 participants
Explanation of Change
Emits a Brotli 11
.brtwin of everyjs/css/html/svg/wasm/ttfasset at build time (viacompression-webpack-plugininconfig/rsbuild/rsbuild.common.ts) and uploads the twins to S3 withContent-Encoding: brand the original'sContent-Type(deploy.yml), so the CDN can serve them to browsers that accept Brotli instead of gzipping on the fly.Steps 1 and 2 of the linked issue. Steps 3 and 4 (CloudFront rewrite to
uri + '.br'forAccept-Encoding: br, Brotli enabled on the Cloudflare zone) are infra changes outside this repo. Until they land, the.brobjects sit unused and nothing user-facing changes.Dependency note:
compression-webpack-pluginis README-deprecated since 2026-09-19compression-webpack-pluginin its README only (the npm package is not flagged, 12.0.0 works as before) and moved compression entirely intominimizer-webpack-plugin≥ 5.11.0 as an asset generator (MinimizerPlugin.compress). That package is the renamedterser-webpack-plugin: 5.6.1 was its last release under the old name.terser-webpack-plugin(the old name) as compatible and its test suite pins^5.6.1. A change in the renamed package on 2026-08-27 (#701, option validation through webpack'svalidatehook, feature-detected withif (compiler.hooks.validate)) breaks it under Rspack, whosecompiler.hooksproxy throws on unknown hook names instead of returningundefined. Verified locally with 5.11.0 on this build: it fails inapplywithCompiler.hooks.validate is not supported in rspack. Using the successor today would need a patch-package patch, and 5.11.0 is additionally blocked by our.npmrcmin-release-age=7until 2026-09-26.compression-webpack-pluginas compatible, so this PR ships with it. Next step: open an issue in Rspack to restore compatibility with the renamed plugin (exposecompiler.hooks.validate), then migrate tominimizer-webpack-pluginin a follow-up.Notes
threshold: 0, minRatio: Infinity- a twin for every matching file..brtwins are excluded from the Workbox precache (the service worker requests the original URLs and the CDN serves the Brotli copy transparently) and from Rsbuild's file-size report.deploy.ymluploads in two passes: everything except*.brexactly as before, then the twins per extension with--content-encoding brand an explicit--content-type(aws-cli would otherwise tag thembinary/octet-stream). The MIME types match what the CDN serves for the originals today (.jsistext/javascript).*.br: that distribution has no Brotli rewrite.Fixed Issues
$ #101250
PROPOSAL:
Tests
npm run build-staging.js/css/html/svg/wasm/ttffile underdist/, including nested ones such ascss/AnnotationLayer.css, has a.brcopy next to it.When deployed to staging:
Offline tests
N/A
QA Steps
No QA
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.