Boost: refactor Critical CSS: Part 1 #22163

pyronaur · 2021-12-22T15:41:15Z

Refactoring the Critical CSS class without modifying the plugin behavior.

It's going to take some time to fully refactor Critical CSS, but this is at least a good first step.

Fixes #21725

Changes in this pull request:

The following functionalities are moved to separate classes/files:
- Critical CSS generation management-related logic.
- Site path providers.
- All Critical CSS REST API logic.
- Critical CSS display in the frontend.
- Recommendations dismissal and reset.
- AMP compatibility is moved to a separate file.
A new base class to handle the options array is added.
Recommendation admin-ajax calls are converted to REST API calls
Admin bar compatibility.
Regeneration admin notices.

Jetpack product discussion

pc9hqz-cz-p2

Does this pull request change what data or activity we track or use?

No

Testing instructions:

Check out this branch - refactor/boost-critical-css
Run jetpack build plugins/boost
- This will install all the dependencies and will build the plugin.
- (This is needed because a lot of class names/filenames have changed.)
Run composer phpcs:lint
- Make sure that there are no phpcs errors under projects/plugins/boost
Run all end to end testing as per the instructions in
- (Handy commands: pnpm run env-start && pnpm run tunnel-on and pnpm run test-e2e)
- Make sure that all the tests pass
Perform extensive manual testing to make sure the plugin works as expected

Signed-off-by: pyronaur <hi@pyronaur.com>

Added Notification base class and extended to Recommendation class to cleanup the current design.

Change module constructors to on_prepare so that the actions calls are not duplicated.

Signed-off-by: pyronaur <hi@pyronaur.com>

…etpack into refactor/boost-critical-css

Signed-off-by: pyronaur <hi@pyronaur.com>

Creating an absolute mess out of the Critical CSS Class. Signed-off-by: pyronaur <hi@pyronaur.com>

Signed-off-by: pyronaur <hi@pyronaur.com>

`! is_admin()` guard clause was protecting against showing Critical CSS anywhere else but the admin :) Signed-off-by: pyronaur <hi@pyronaur.com>

Signed-off-by: pyronaur <hi@pyronaur.com>

Move providers to `critical-css/path-providers/providers` Signed-off-by: pyronaur <hi@pyronaur.com>

Signed-off-by: pyronaur <hi@pyronaur.com>

This is not a permanent fix, but I just need e2e tests to run right now

Add an interface and a class to ensure that a class is going to need to be set-up at a later point in time via hook.

thingalon · 2022-01-28T00:50:26Z

projects/plugins/boost/app/admin/class-admin.php

@@ -54,21 +54,13 @@ class Admin {
 	 */
 	private $speed_score;

-	/**
-	 * Initialize the class and set its properties.


😲 Is THAT what a constructor does?!

…ting interfaces without warty comments

…he UI doesn't freeze if the server 500s

thingalon

I made a couple of improvements:

b7401e5 adds an exception to the "unused variable" rule, to allow any that start with an _. That way we won't need linter warts any time an interface implementor doesn't want to use a function parameter specified by the interface.
8893c9c ensures 500 errors which occur during critical css generation initialization aren't ignored (one crashed the UI during my testing)

This is beautiful work. I particularly love the interfaces, and how they are implemented and used. Let's get this thing merged.

thingalon · 2022-01-28T00:52:22Z

projects/plugins/boost/app/assets/src/js/stores/modules.ts

@@ -9,14 +9,24 @@ import { writable } from 'svelte/store';
 import config from './config';
 import { setModuleState } from '../api/modules';

+export type Optimizations = {


Is Optimization a synonym for Module?

Optimization is a feature, but a feature doesn't necessarily mean optimization.

This came out with a brainstorming call with @eAdnan007 where we had this idea that Boost has other "features" that aren't optimizations, and so they're hanging in the air somewhere.

I actually think this is going to need a follow-up PR to get rid of the "Module" terminology entirely, at least when it comes to describing Optimizations and Features. I think that distinction makes it a lot easier to reason about what belongs where.

That seems reasonable to me, and I'm fine towards replacing Module with Feature / Optimization.

thingalon · 2022-01-28T00:52:58Z

projects/plugins/boost/app/contracts/Feature.php

@@ -0,0 +1,11 @@
+<?php
+
+namespace Automattic\Jetpack_Boost\Contracts;


I really like this namespace and what it represents.

thingalon · 2022-01-28T00:55:20Z

projects/plugins/boost/app/features/optimizations/critical-css/Critical_CSS.php

+use Automattic\Jetpack_Boost\REST_API\Endpoints\Recommendations_Reset;
+use Automattic\Jetpack_Boost\REST_API\REST_API;
+
+class Critical_CSS implements Feature, Has_Endpoints {


I really, really love this contract syntax which tells you in the header some common ways you can interact with the class.

thingalon · 2022-01-28T00:59:29Z

projects/plugins/boost/app/rest-api/endpoints/Generator_Request.php

+use Automattic\Jetpack_Boost\REST_API\Contracts\Endpoint;
+use Automattic\Jetpack_Boost\REST_API\Permissions\Current_User_Admin;
+
+class Generator_Request implements Endpoint {


I think it would be nice to include a comment with each endpoint which says its HTTP method, path, and purpose. e.g.:

/*** * POST critical-css/request-generate * * Ask the plugin to start a new Critical CSS Generation process. Returns the updated state of the Critical CSS system. */

Just to make it easier when browsing endpoints.

👍 That makes sense

thingalon · 2022-01-28T02:52:49Z

projects/plugins/boost/app/modules/lazy-images/class-lazy-images.php

-	 * Check whether the Jetpack Lazy Images module is enabled.
-	 */
-	public static function is_jetpack_lazy_images_module_enabled() {
-		return class_exists( 'Jetpack' ) && Jetpack::is_module_active( self::MODULE_SLUG );


Ok I just imported my changes from my fixes branch, and there are two tests failing. It is not keeping the "lazy-images" feature in lock-step between Jetpack and Boost. This line is why -- the Modules used to be able to override the default status behavior.

I started down the road to fix this -- I created a new JetpackDeferredStatus class which inherits from Status and overrides its is_enabled and update methods to defer to Jetpack's module status.

But then how do we make some Optimizations use Status, and others use JetpackDeferredStatus?

I could see a few solutions. Among them, this one felt the least messy to me:

Change Feature from an interface to a base class, so that it can define default methods,

Add a create_status_manager method to the Feature base class, which returns a new Status object, and override it for lazy images to return a new JetpackDeferredStatus object instead.

Profit!

But that's a big enough design change that I didn't want to do it without consulting with @pyronaur first.

Alternately, we could:

Add a FeatureDefaults base class which each feature inherits in addition to using the Feature interface, so that we have somewhere to park defaults which apply to most features - but also maintain the clean Feature contract.

Add a create_status_manager method to every feature. That's kind of tiresome.

Add an empty interface like UsesJetpackStatus to tag the Lazy_Images class as wanting to defer its status to Jetpack, and checking for it in the Optimization class constructor. But that feels like magic-at-a-distance which will trip us up later.

Add the deferral logic to the Status class, and have it know a bunch of slugs to defer to Jetpack's status. That feels like it puts the intelligence in the wrong place.

Add a special case to the Optimization class and have it know which Features need special status treatment. Same problem with intelligence going in the wrong place.

None of those options seem clean to me.

So @pyronaur, how would you go about this?

I started down the road to fix this -- I created a new JetpackDeferredStatus class which inherits from Status and overrides its is_enabled and update methods to defer to Jetpack's module status.

This is certainly one way of doing things. Although, whenever possible - I think we should try to think how we can do this with composition first (instead of inhertiance)

But then how do we make some Optimizations use Status, and others use JetpackDeferredStatus?

Not entirely sure either, but I think it's worth exploring adding a status contract where both Jetpack_Status and Boost_Status both subscribe to that contract and become interchangeable. At that point, whoever is instantiating a status can decide which type of status is needed (for example, depending on whether Jetpack is active)

But there's a problem with this approach still - because both plugins can be activated without the other, so if the place of storage changes, there's no easy way to silently sync and migrate the option.

I have to look closer at the lazy images module - but I think maybe that module itself should be able to know whether it's active or not? I'm not sure, but it would be a way to neatly fix this.

Alternately, we could:

Hm. I need to do a little bit of thinking on this, but it seems that if we use any type of inheritance it's automatically going to tie down the parent class.

Thinking from a single-responsibility angle - what's the single responsibility we're missing here? Sounds like we're talking about where to store the status - either in Jetpack options or Boost options. So I think that's the hint of the direction we need to go in.

No other classes should be aware that we have a compatibility layer with Jetpack. Where to place it exactly - I'm not entirely sure. I think it needs a bit of thought as a part of #22542

Boost modules were moved to a features/ directory in #22163, but this task wasn't updated accordingly. This should fix things.

karthikax and others added 30 commits December 7, 2021 17:13

Move Critical CSS Recommendation to own class

4df09b6

Add changelog

52c8893

After recommendation refactor fix

51bb0c9

Signed-off-by: pyronaur <hi@pyronaur.com>

Move constants filter inside CSS Recommendations class

b9d596c

Signed-off-by: pyronaur <hi@pyronaur.com>

Move on_uninstall

69c188c

Signed-off-by: pyronaur <hi@pyronaur.com>

Refactor Critical CSS recommendation

9a93e71

Added Notification base class and extended to Recommendation class to cleanup the current design.

Remove legacy Recommendation

9a2bf24

Refactor get notifications for abstraction

3cd1b51

Refactor inheritance aware notification key

64cbf72

Refactor module constructors to on_prepare

910c975

Change module constructors to on_prepare so that the actions calls are not duplicated.

Remove non-existing class from comment

b2be2b9

Signed-off-by: pyronaur <hi@pyronaur.com>

Refactor Notifications to OptionsArray

2ee198c

Merge branch 'refactor/boost-critical-css' of github.com:Automattic/j…

29ce1d2

…etpack into refactor/boost-critical-css

Dump generation related functions into Generator class

c01f6c6

Signed-off-by: pyronaur <hi@pyronaur.com>

Go ahead, make a mess.

6ce9cdd

Creating an absolute mess out of the Critical CSS Class. Signed-off-by: pyronaur <hi@pyronaur.com>

Re-add handle_css_proxy

1849901

Signed-off-by: pyronaur <hi@pyronaur.com>

Move: api_get_critical_css_status

48a9159

Signed-off-by: pyronaur <hi@pyronaur.com>

Add critically bad comments

5c916e2

Signed-off-by: pyronaur <hi@pyronaur.com>

Remove module is_initialized

8f823d4

Clarify method name: get_current_request_css

dfa4930

Signed-off-by: pyronaur <hi@pyronaur.com>

Create a dedicated method to display Critical CSS

71b7e0f

Signed-off-by: pyronaur <hi@pyronaur.com>

Whoops - wrong condition.

f0fec8c

`! is_admin()` guard clause was protecting against showing Critical CSS anywhere else but the admin :) Signed-off-by: pyronaur <hi@pyronaur.com>

Move & De-duplicate AMP Compatibility

831d997

Signed-off-by: pyronaur <hi@pyronaur.com>

Refactor AMP conditions to separate file

2af3a56

Move Providers

12b2622

Move providers to `critical-css/path-providers/providers` Signed-off-by: pyronaur <hi@pyronaur.com>

Create a class to manage Critical CSS Paths

12dbf1b

Signed-off-by: pyronaur <hi@pyronaur.com>

Add module initialized action

3ce3ea2

Move generator out of the main Critical CSS class

60451d4

Signed-off-by: pyronaur <hi@pyronaur.com>

Fix missing providers

8dc42b5

Signed-off-by: pyronaur <hi@pyronaur.com>

Remove the need to inject providers

5fc4d5e

Signed-off-by: pyronaur <hi@pyronaur.com>

jeherve assigned pyronaur Jan 20, 2022

pyronaur added 8 commits January 20, 2022 17:02

Simplify uninstall

4aaa81f

Add missing namespaces

1352b1d

Fix CLI for e2e tests

9226329

This is not a permanent fix, but I just need e2e tests to run right now

Seamlessly migrate to the new options format.

c7c69b2

Protect against recursion

967ce52

Add Has_Setup and Setup classes

2a28b05

Add an interface and a class to ensure that a class is going to need to be set-up at a later point in time via hook.

Improve how Optimization objects are initialized

1f32532

[not verified] Fix: return statement was left over after refactoring

507e4f0

thingalon reviewed Jan 28, 2022

View reviewed changes

Mark George added 2 commits January 28, 2022 12:28

Allow any unused parameters which start with _, to allow for implemen…

b7401e5

…ting interfaces without warty comments

Catch errors that occur when queuing up Critical CSS generation, so t…

8893c9c

…he UI doesn't freeze if the server 500s

thingalon approved these changes Jan 28, 2022

View reviewed changes

thingalon requested changes Jan 28, 2022

View reviewed changes

thingalon previously approved these changes Jan 28, 2022

View reviewed changes

thingalon mentioned this pull request Jan 28, 2022

Reinstate Jetpack / Boost Lazy-Loading sync #22542

Closed

Mark George added 2 commits January 31, 2022 14:26

Merge branch 'master' into refactor/boost-critical-css

970f6e3

Skip lazy-image tests until reinstated later

ce453af

thingalon dismissed their stale review via ce453af January 31, 2022 04:01

github-actions bot added the E2E Tests label Jan 31, 2022

thingalon approved these changes Jan 31, 2022

View reviewed changes

thingalon enabled auto-merge (squash) January 31, 2022 04:09

thingalon merged commit 1af92d6 into master Jan 31, 2022

thingalon deleted the refactor/boost-critical-css branch January 31, 2022 04:14

github-actions bot removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Status] In Progress labels Jan 31, 2022

github-actions bot added this to the boost/1.3.2 milestone Jan 31, 2022

karthikax mentioned this pull request Jan 31, 2022

Boost: Refactor Critical CSS class #21845

Closed

jeherve added a commit that referenced this pull request Feb 9, 2023

Repo gardening: update auto-labeling rules for Boost

92f2926

Boost modules were moved to a features/ directory in #22163, but this task wasn't updated accordingly. This should fix things.

jeherve mentioned this pull request Feb 9, 2023

Repo gardening: update auto-labeling rules for Boost #28871

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Boost: refactor Critical CSS: Part 1 #22163

Boost: refactor Critical CSS: Part 1 #22163

pyronaur commented Dec 22, 2021 •

edited by karthikax

Loading

thingalon Jan 28, 2022

thingalon left a comment

thingalon Jan 28, 2022

pyronaur Jan 28, 2022

thingalon Jan 28, 2022

thingalon Jan 28, 2022

thingalon Jan 28, 2022

thingalon Jan 28, 2022

pyronaur Jan 28, 2022

thingalon Jan 28, 2022 •

edited

Loading

pyronaur Jan 28, 2022

		@@ -0,0 +1,11 @@
		<?php

		namespace Automattic\Jetpack_Boost\Contracts;

Boost: refactor Critical CSS: Part 1 #22163

Boost: refactor Critical CSS: Part 1 #22163

Conversation

pyronaur commented Dec 22, 2021 • edited by karthikax Loading

Changes in this pull request:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

Choose a reason for hiding this comment

thingalon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thingalon Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pyronaur commented Dec 22, 2021 •

edited by karthikax

Loading

thingalon Jan 28, 2022 •

edited

Loading