wp-calypso icon indicating copy to clipboard operation
wp-calypso copied to clipboard

Plugins Management: Use the new endpoint for fetching all site plugins

Open vitozev opened this issue 2 years ago • 6 comments

Proposed Changes

In https://github.com/Automattic/wp-calypso/pull/61803, @enejb did a fantastic job using react-query for some plugin endpoints, but IMO we should try to split such refactor into much smaller pieces to avoid the possibility of introducing a regression.

Migrating all plugins to react-query would require us to:

  • Research if we can replace Redux completely. In a quick chat, @tyxla pointed out that there might be some dynamics in the UI (e.g., clicking "Activate plugin" would be something we currently can't track on the server side, but it's available in the store).
  • Define the phases of how this should happen.
  • Work on small incremental steps towards the goal.

What about this PR?

As properly migrating to react-query would require more time, this changeset introduces a new query component, QueryAllJetpackPlugins that is used in all places where we need to fetch installed plugins for all Jetpack sites. The existing <QueryJetpackPlugins /> component will still be used for fetching plugins for a single Jetpack site.

We found some inconsistencies, which were documented in: pbtFFM-29g-p2

Note: I'm working on some tests, but I'd like to get this code reviewed while working on them.

Testing Instructions

  • Checkout PR locally
  • Run yarn start
  • When the build is ready, run yarn start-jetpack-cloud-p
  • Visit http://calypso.localhost:3000/plugins/manage and verify that the screen loads as expected (hopefully more instantly as well)
  • Verify that results match the ones from the production environment. If they don't match, this shouldn't be directly related to this PR, but would be good to investigate the case further.
  • Verify that bulk actions (install, activate, enable auto-update, update and remove plugins) still work as expected.
  • Visit http://calypso.localhost:3000/plugins/akismet and verify that the screen loads as expected.
  • Visit http://calypso.localhost:3000/plugins/browse/popular and verify that the screen loads as expected.
  • Visit http://jetpack.cloud.localhost:3001/plugins/manage and verify that the screen loads as expected (and matches what you see on Calypso Blue)
  • Visit http://jetpack.cloud.localhost:3001/plugins/akismet and verify that the screen loads as expected.
  • Visit http://jetpack.cloud.localhost:3001/plugins/manage/<SITE> and verify that the screen loads as expected.

Pre-merge Checklist

  • [ ] Have you written new tests for your changes?
  • [ ] Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • [ ] Have you checked for TypeScript, React or other console errors?
  • [ ] Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • [x] Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?

Related to 1202518759611394-as-1202534044814282

vitozev avatar Aug 08 '22 12:08 vitozev

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~31 bytes added 📈 [gzipped])

name           parsed_size           gzip_size
entry-stepper       +124 B  (+0.0%)      +31 B  (+0.0%)
entry-main          +124 B  (+0.0%)      +31 B  (+0.0%)
entry-login         +124 B  (+0.0%)      +31 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~483 bytes added 📈 [gzipped])

name                                 parsed_size           gzip_size
plugins                                  +1140 B  (+0.1%)     +275 B  (+0.1%)
jetpack-cloud-plugin-management          +1076 B  (+0.1%)     +269 B  (+0.1%)
stats                                     +473 B  (+0.1%)     +139 B  (+0.1%)
settings-performance                      +473 B  (+0.1%)     +139 B  (+0.1%)
plans                                     +473 B  (+0.1%)     +139 B  (+0.1%)
marketplace                               +473 B  (+0.1%)     +139 B  (+0.1%)
marketing                                 +473 B  (+0.1%)     +139 B  (+0.1%)
email                                     +473 B  (+0.1%)     +139 B  (+0.1%)
activity                                  +473 B  (+0.1%)     +139 B  (+0.1%)
woocommerce-installation                  +118 B  (+0.0%)      +30 B  (+0.0%)
themes                                    +118 B  (+0.0%)      +31 B  (+0.0%)
sites-dashboard                           +118 B  (+0.1%)      +30 B  (+0.0%)
site-purchases                            +118 B  (+0.0%)      +31 B  (+0.0%)
settings                                  +118 B  (+0.0%)      +27 B  (+0.0%)
scan                                      +118 B  (+0.0%)      +31 B  (+0.0%)
purchases                                 +118 B  (+0.0%)      +31 B  (+0.0%)
hosting                                   +118 B  (+0.0%)      +30 B  (+0.0%)
home                                      +118 B  (+0.0%)      +28 B  (+0.0%)
backup                                    +118 B  (+0.0%)      +31 B  (+0.0%)
automated-transfer-state-middleware       +118 B  (+0.6%)      +30 B  (+0.5%)

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 (~314 bytes added 📈 [gzipped])

name                                                 parsed_size           gzip_size
async-load-design-playground                              +473 B  (+0.0%)     +139 B  (+0.0%)
async-load-design                                         +473 B  (+0.0%)     +139 B  (+0.0%)
async-load-calypso-my-sites-sidebar                       +201 B  (+0.2%)      +58 B  (+0.2%)
async-load-calypso-layout-guided-tours-component          +201 B  (+0.2%)      +54 B  (+0.2%)
async-load-calypso-lib-preferences-helper                 +124 B  (+0.1%)      +33 B  (+0.1%)
async-load-signup-steps-woocommerce-install-confirm       +118 B  (+0.1%)      +30 B  (+0.1%)

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. Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

matticbot avatar Aug 08 '22 12:08 matticbot

This PR modifies the release build for editing-toolkit

To test your changes on WordPress.com, run install-plugin.sh editing-toolkit fix/use-new-me-sites-plugins-endpoint on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-mMA-p2

matticbot avatar Aug 09 '22 09:08 matticbot

Verify that results match the ones from the production environment. If they don't match, this shouldn't be directly related to this PR, but would be good to investigate the case further.

I know this is unlikely to be related to these changes, but I'm describing it here so that it can be investigated further.

The number of plugins doesn't match. In concrete, I see two Jetpack plugins that are not visible in production (see the ones highlighted in a red box). I haven't checked the code, but maybe we are hiding/skipping some specific plugins.

image

rcanepa avatar Aug 09 '22 16:08 rcanepa

After a quick code review I can say that the changes look reasonable and easy to follow. Furthermore, features seem to work as expected as well. However, I'll spend more time tomorrow reviewing everything before giving an official review.

rcanepa avatar Aug 09 '22 18:08 rcanepa

@rcanepa, thank you for your review! I added one card to investigate why some test plugins are returned from the new endpoint.

In short, if you open your mobile app, you most likely will see the same plugin list as the one returned by our new endpoint.

Card: 1202518759611394-as-1202882177960021

vitozev avatar Aug 29 '22 11:08 vitozev

@tyxla, thank you for all your great feedback here! I think I've addressed everything. Let me know if you have the bandwidth to do a quick review of everything that was changed before we proceed with the merging of this PR.

FWIW, we will write some new unit tests in a separate PR in the following days.

vitozev avatar Aug 29 '22 11:08 vitozev

@tyxla, thank you for all your great feedback here! I think I've addressed everything. Let me know if you have the bandwidth to do a quick review of everything that was changed before we proceed with the merging of this PR.

FWIW, we will write some new unit tests in a separate PR in the following days.

I can take another look at it tomorrow if that is not too late for you folks.

tyxla avatar Aug 29 '22 11:08 tyxla