-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Ensure stepper declarative flow components tree-shake properly #90851
Ensure stepper declarative flow components tree-shake properly #90851
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~159537 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~270093 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~5216 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I'm the right person to evaluate the correctness of this since I'm totally unfamiliar with the packages here, but following up on the performance discussion we had (and I'm going to repeat myself here and paste in some numbers from our 1 on 1 discussion for anyone else who wants to follow the discussion).
we have the following from the unused JS coverage in chrome dev tools going from /start
to the design picker (for going through one flow — the true percentage of useful JS is better than this because users might do interactions that trigger functions that weren't used here, and these numbers should be uncompressed sizes, and also include some stuff like the React dev tools chrome extension, which isn’t really a WP thing, but is identical in both):
- Prod:
3MB
used of8.7MB (5.7MB unused)
. - This version:
2.9MB
used of8.4MB (5.5MB unused)
.
I’m hoping (?) the drop in used JS is because I ran the Calypso Live version, which doesn’t seem to emit tracks events or emits fewer tracks is events and is maybe missing some other stuff, and not because setting "sideEffects": false
actually caused a change.
In terms of measured performance, we had a 15s LCP
at the start of the onboarding flow (at /start
) on a Tecno Spark 8C with "Fast 3G" set in chrome dev tools. I tried measuring this a few more times and it’s moderately noisy and tends to land between 13.5s and 15s
on the prod version. I tried this again with the Calypso Live version and saw basically the same numbers. It seems like this should be strictly better, so there should be an improvement here or somewhere else in the flow, but perhaps it’s small enough that we’d have to aggregate more runs to see the improvement.
I didn’t run through the whole flow because it’s fairly slow on a slow device and is resource intensive enough that it sometimes crashes, making it even slower, but for reference, the old version intent page had a 14s LCP
and the design picker (business tab) had an 85s LCP
(all of these “before” numbers are without #90741 but with #88786 and #88925).
I'm going to go ahead and merge this, as this is a no-code change, and smoke-testing seems to be safe. |
Thank you for the review, @danluu! 👍
Yes, that sounds about right. I don't think it's easy to create a live branch for the branching point of a PR any more, otherwise we could double-check.
Right, this is one of those "all other things being equal, fewer bytes is better" changes. |
This reverts commit 33d1e85.
Revert it as some styles are not loaded. See p1716262266425289-slack-C02FMH4G8. |
Sorry for the breakage, and thank you for handling the revert, @arthur791004 ! |
This PR enables tree-shaking for the stepper declarative flow internal components sub-tree, so that only the required ones get put in the entry point chunk.
This appears to result in a massive reduction (160KB gzipped / over 500KB uncompressed) of the stepper entry-point, with the affected components moving instead to more specific chunks such as individual flows.
Proposed Changes
package.json
with"sideEffects": false
to the stepper declarative flow internal components sub-treeWhy are these changes being made?
Testing Instructions
Smoke-test a few stepper flows and ensure that nothing breaks.
Pre-merge Checklist