-
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
Restricting fetchModuleList calls to Atomic sites only #91170
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 (~451 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 (~43 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.
Change seem reasonable and the extra call is not made.
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 believe we need to consider non-Atomic Jetpack sites in the equation as well.
|
||
const request = ( siteId ) => ( dispatch, getState ) => { | ||
if ( siteId && ! isFetchingJetpackModules( getState(), siteId ) ) { | ||
const isAtomic = isSiteWpcomAtomic( getState(), siteId ); |
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.
What about non-atomic Jetpack sites? From what I recall, Jetpack sites (which aren't necessarily Atomic sites) also used the module data - extensively in site settings (but not only there).
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.
if ( siteId && ! isFetchingJetpackModules( getState(), siteId ) ) { | ||
const isAtomic = isSiteWpcomAtomic( getState(), siteId ); | ||
|
||
if ( siteId && ! isFetchingJetpackModules( getState(), siteId ) && isAtomic ) { |
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.
It can be convenient to add this site type limitation here, but it also means we have to test every combination of the following:
- Area/component where the
QueryJetpackModules
query component is used - Site type that supports (or doesn't support) Jetpack modules - Atomic sites, non-Atomic Jetpack sites, simple sites, etc.
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.
Area/component where the QueryJetpackModules query component is used
I tested it on its usages, and they are working as expected:
- Pages -> All pages
- Settings -> Newsletter
- Tools -> Marketing
Jetpack modules - Atomic sites, non-Atomic Jetpack sites, simple sites, etc.
- Atomic sites and non-Atomic Jetpack sites are working as expected after this change.
- Simple site is not calling it as expected.
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
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.
Left a couple more nits on the unit tests, but nothing critical - this looks good to go 🚀
isWpcomAtomic = true; | ||
isJetpack = false; |
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.
Doesn't seem like a valid scenario. AFAIK there can't be a working Atomic site that isn't a Jetpack site under the hood.
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.
That makes sense, updated it here @tyxla.
@@ -49,6 +51,9 @@ function getStore() { | |||
|
|||
describe( 'fetchModuleList', () => { | |||
test( "Ensure we're NOT calling fetchModuleList for simple sites", async () => { | |||
isWpcomAtomic = false; | |||
isJetpack = false; |
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.
If we always declare these in the beginning of the test, we might as well remove the let
call and move them here as const
s?
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'm not sure if I understood your suggestion since we're using these variables inside mockStore
, and if we declare them constants, we can't change them on other tests.
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 see, that makes sense. Let's leave them as they are then 👍
Related to #90758
Proposed Changes
fetchmoduleList
call for Atomic sites onlyHere's more context to it.
Why are these changes being made?
Testing Instructions
Home
on that simple site and clear your Network requestsPages -> All pages
/rest/v1.1/jetpack-blogs
API callPre-merge Checklist