Remove postcss from dist/record.js (it was only intended for replay)#1784
Remove postcss from dist/record.js (it was only intended for replay)#1784eoghanmurray wants to merge 13 commits intorrweb-io:masterfrom
Conversation
…output - postcss is only needed at record time
…t; we use a stub here to ensure thta the `hasShadowRoot` function still works (`n instanceof BaseRRNode` will always be false)
🦋 Changeset detectedLatest commit: 2b78153 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Size Change: -823 kB (-8.15%) ✅ Total Size: 9.27 MB
|
eoghanmurray
changed the title
… a comment to the PR" That didn't work, see preactjs/compressed-size-action#2
…_request_target` permission which will allow the check to write back into the PR directly Q: are there any new 'heavy' steps added here in comparison to where it has been moved from? Claude AI: No. The compressed-size-action does its own install and build internally via the install-script and build-script parameters — it checks out both the base and PR branches, installs, and builds each to compare sizes. That's the same work it was doing in ci-cd.yml. The only new steps are actions/checkout and actions/setup-node, which are lightweight. In fact, in ci-cd.yml it was arguably more wasteful — the job had already done a full yarn install and yarn build:all before the action ran its own install+build again via the parameters.
…-io#1784 - adding here also to show how the conflicts would resolve
There was a problem hiding this comment.
Let's try and generalize the changes here and move them to /vite.config.default.ts.
One of the reasons to move towards a unified build process was to reduce package specific bundle code.
There was a problem hiding this comment.
Right, but I think this could also be solved with a more in-depth refactor of rrweb-snapshot, to make a cleaner separation of snapshot and rebuild functionality (specifically not having a shared utils.ts for both sides)
The packages/record and packages/replay are intended to be the final destination for the packages/rrweb/src/record and packages/rrweb/src/replay code, so that could help too in that we could then remove this 'hack'. From that pov I'd vote to go forward with the exceptions in this PR only living in the ./packages/record folder and checking after those refactors whether we can remove
There was a problem hiding this comment.
So in latest the `rollupOptions are gone (commented out) from record/vite.config.ts
That just leaves recordOnlyResolvePlugin as the main customization in that config.
Rollup plugin that redirects workspace package imports to their source entry
points during JS bundling. This lets Rollup see individual source modules
and tree-shake out replay-only code (rebuild.ts / postcss).
Is this a candidate for moving to the top level? As I understand it, it would need to be rewritten as 'resolve to individual source modules instead of the 'barrel' outputs? Is this a known technique?
| // Allow Rollup to drop modules whose exports are unused, even | ||
| // if they contain top-level code (e.g. rebuild.ts / postcss). | ||
| moduleSideEffects: false, |
There was a problem hiding this comment.
How do we know this blanket statement doesn't have any unintended consequences?
There was a problem hiding this comment.
I had AI go through the whole repo, it seems like the risk is mostly minimal/hypothetical currently.
There was a problem hiding this comment.
I moved it up to root to see how it affects build for all projects;
It won't work for rrweb-player as it results in the Sveldt library being dropped from the output:
You get a diff e.g. in rrweb-player.cjs where the following bit is removed:
const PUBLIC_VERSION = "4";
if (typeof window !== "undefined")
(window.__svelte || (window.__svelte = { v: new Set() })).v.add(PUBLIC_VERSION);
[+ many many lines]
Presumably that's Svelte making itself available (a side effect!).
There are also changes to the chrome extension, presumably because it pulls in rrweb-player but I didn't check
There was a problem hiding this comment.
One other change (maybe minor, maybe not) which affects all build outputs in all packages is the addition of a /* @__PURE */ marker to certain parts, e.g. in packages/all/dist/all.cjs:
var stringify_1$1;
var hasRequiredStringify$1;
function requireStringify$1() {
if (hasRequiredStringify$1) return stringify_1$1;
hasRequiredStringify$1 = 1;
let Stringifier = /* @__PURE__ */ requireStringifier$1();
function stringify(node2, builder) {
let str = new Stringifier(builder);
str.stringify(node2);
}
stringify_1$1 = stringify;
stringify.default = stringify;
return stringify_1$1;
}
var node$1;
var hasRequiredNode$1;
function requireNode$1() {
if (hasRequiredNode$1) return node$1;
hasRequiredNode$1 = 1;
let { isClean, my } = /* @__PURE__ */ requireSymbols$1();
let CssSyntaxError = /* @__PURE__ */ requireCssSyntaxError$1();
let Stringifier = /* @__PURE__ */ requireStringifier$1();
let stringify = /* @__PURE__ */ requireStringify$1();
function cloneNode(obj, parent) {
let cloned = new obj.constructor();
for (let i2 in obj) {
if (!Object.prototype.hasOwnProperty.call(obj, i2)) {
continue;
I don't know if this is also a problem?
There was a problem hiding this comment.
How do we know this blanket statement doesn't have any unintended consequences?
We know from checking (at this point in time of the codebase) that it doesn't have any unintended consequences for the record package — perhaps it might have an unintended consequence in future, but that will be discovered as part of any PR that introduces a side-effecting-module for the record side by the record/dist/ output not working (assuming we are using the record package for testing). I think this could be a good thing that we aggressively tree shake packages/record by default?
For now anyhow I've removed this blanket statement, but moved it into the rrweb-snapshot package.json instead as sideEffects: false there.
I've added notes in record/vite.config.ts and also in rrweb-snapshot/BUILD.md to document this.
…ackage so that it's more targeted - that's the only material affect the blanket version was affecting
…1787) * Update filesize badges (might need further evolution before 2.0.0) * Don't run full CI/CD when only .md docs have changed in the PR - move eslint checks into their own file so they can also ignore .md changes - prettier checks don't need the same perms as eslint, so we can demote pull_request_target -> pull_request * Add empty changeset * Implement the bundle size change originally originally added in #1784 - adding here also to show how the conflicts would resolve * Update .github/workflows/eslint-check.yml --------- Co-authored-by: Justin Halsall <Juice10@users.noreply.github.com>
|
Size Change: -1.06 MB (-10.24%) 👏 Total Size: 9.27 MB
ℹ️ View Unchanged
|
2 participants
This succeeds #1743
Thanks to #1630 we should be able to see the effect of this in the CI as part of this PR
Tree-shaking postcss
The
rrweb-snapshotpackage contains both snapshot (record-time) and rebuild(replay-time) code. The rebuild module imports
postcss, a large dependencythat is only needed during replay. Because
rrweb-snapshotre-exportseverything from a single
index.tsbarrel file, bundlers that consume thepre-built package see it as one indivisible chunk — they cannot tree-shake out
rebuild.tsand itspostcssimport, even when only snapshot functions areused.
We initially tried adding
/*#__PURE__*/annotations to side-effectingexpressions (e.g.
new RegExp(...)) insiderrweb-snapshot/src/css.ts, hopingRollup would then drop the unused modules. This did not work because by the
time this package imports from the published
rrweb-snapshotpackage, thesources have already been bundled together — the individual module boundaries
that Rollup needs for tree-shaking no longer exist.
The solution (in
vite.config.ts) uses two techniques:A custom
resolveIdplugin redirects imports ofrrweb,rrweb-snapshot,and
rrdomto their original source entry points instead of thepre-built packages. This restores individual module boundaries so Rollup can
see exactly which source files are used and which are not.
treeshake.moduleSideEffects: false"sideEffects": false,in rrweb-snapshot/package.json] tells Rollup it is safe to dropany module[the rrweb-snapshot modules] whose exports are not consumed, even if that module containstop-level code. This allows
rebuild.ts(and thereforepostcss) to beeliminated entirely from the record bundle.
Additionally,
packages/rrweb/src/entries/record.tswas changed from adefault-import-then-re-export pattern to direct named re-exports, which gives
Rollup a clearer view of what is actually used.
A test in
packages/record/test/record.test.tsasserts that no output bundlefile contains the string "postcss", guarding against future regressions.
Co-Authored by Claude AI