-
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
Settings: Fix useSiteCopy
features requesting selector
#98583
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: Sections (~6 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. 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.
Looks good. But an even better solution will be to finish #94840 🙂
The features.active
field is already available on the site object, the features don't need to be fetched separately. #94840 also modifies useSiteCopy
, you can see there how the code should eventually look like.
It turns out that the approach in #94840 is probably too ambitious: modify the siteHasFeature
/getSiteFeatures
selector, update the 30+ usage places to stop fetching, and test that none of them are broken.
We'll be better of doing something else: introduce new selectors, like siteHasFeatureEx
and getSiteFeaturesEx
, and start using these selectors at one place at a time. And do the migration gradually.
I agree 👍 One step at a time, and I always prefer small steps. Thanks for taking a look! |
Proposed Changes
Updates
useSiteCopy
hook to use theisRequestingSiteFeatures
selector instead of building a custom one and returning a new object on every run.This also improves the code quality: we already have a dedicated selector for this, so we should be using it.
Introduced previously in #72714.
Why are these changes being made?
To reduce the number of rerenders on the Settings > General page and resolve this warning:
Testing Instructions
/settings/general/:site
where:site
is a WP.com simple site./setup/copy-site/domains?sourceSlug=nonexistingweirdsite.wpcomstaging.com
as per Copy site: add flow validation to check ifsourceSlug
is a valid site #72714./sites
is expected.