Skip to content

Create Enqueued_Scripts_Size_Check #17

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

Closed
Tracked by #3
mehulkaklotar opened this issue Nov 21, 2022 · 9 comments · Fixed by #76
Closed
Tracked by #3

Create Enqueued_Scripts_Size_Check #17

mehulkaklotar opened this issue Nov 21, 2022 · 9 comments · Fixed by #76
Assignees
Labels
Checks Audit/test of the particular part of the plugin [Type] Enhancement A suggestion for improvement of an existing feature
Milestone

Comments

@mehulkaklotar
Copy link
Member

mehulkaklotar commented Nov 21, 2022

Description

Checks the number of scripts and the total size of all scripts registered in the plugin. This check will run the wp_enqueue_scripts action to register all plugin scripts. Using wp_scripts(), all scripts registered by the plugin will be looped over to get the size of the script along with any additional inline scripts. The number and total file size of all scripts will be calculated and then tested against the threshold, warning if either exceeds a threshold. Check should run relevant actions to make the data as accurate as possible as scripts can be registered and enqueued with any actions provided by WP like in the header or in the document body. Check should run through multiple contexts to make sure scripts enqueued by the plugin are covered in the analysis. Contexts could be any URLs from the site like the homepage, post types, templates, etc.

This issue is dependant on:

Acceptance Criteria

Enqueued_Scripts_Size_Check class

  • Class Enqueued_Scripts_Size_Check should be created and exists at includes/Checker/Checks
  • Class Enqueued_Scripts_Size_Check should extend the Abstract_Runtime_Check class.
  • Class Enqueued_Scripts_Size_Check will use the URL_Aware trait.
  • Class Enqueued_Scripts_Size_Check will implement theWith_Shared_Preparations interface.
  • Class has a protected $threshold_size property with a value of 300000.
  • A prepare() method will be implemented in the class.
    • This method will save the $GLOBALS['wp_scripts']; to a temporary variable.
    • Then set the $GLOBALS['wp_scripts']; to null if set.
    • A clean up function is returned which uses the temporary variable to reinstate the original $GLOBALS['wp_scripts']; value.
  • A get_shared_preparations() method will be implemented.
    • The get_shared_preparations() method will be used to return a preparation that creates a demo post for all publicly viewable post types using Create Demo_Posts_Creation_Preparation #40
    • The method will return an array where the Demo_Post_Creation_Preparation class name is an array key and it's value is an array of posts data to create, compatible with wp_insert_post.
  • A protected get_urls() method will be created.
    • The method will return an array of URLs containing the home URL and URLs for all publicly viewable post types.
  • Method run( Check_Result $results ) should be implemented from the Runtime_Check abstract class.
    • The run() method will call the run_for_urls() method passing an array of URLs returned by the get_urls() method, a callable which will be the classes check_url() method, and the Check_Results instance.
    • The Check_Result $check_result will be returned by the run method.
  • A protected check_url() method will be created.
    • The method will accept an instance of the Check_Result class and a URL string as parameters.
    • The method will reset the $GLOBALS['wp_scripts'];
    • Then will call the wp_enqueue_scripts() to enqueue all of the checked plugins scripts.
    • Using the object returned by wp_scripts() all enqueued scripts for the URL will be looped over:
      • Check the script is part of the plugin being checked by matching the plugin URL with the script URL
      • If the script is part of the plugin, calculate the size of the script including any additional inline scripts.
      • Add the script path and size to a $plugin_scripts array to be used as part of the results.
      • Record the total plugin scripts size in a $plugin_scripts_size variable to be used as part of the results.
    • After looping over all plugin scripts, the total $plugin_scripts_size is checked agains the $threshold_size
    • If the total script size is larger than the threshold, warnings are added to the $check_results
      • Each script in the $plugin_scripts is looped over an a warning message added for each.
      • Each warning includes the current URL, the threshold, the script path and size.

URL Aware Trait

  • A new URL_Aware trait exists in includes\Traits\URL_Aware
  • Trait contains a public go_to( $url ) method that will simulate in WordPress the passed URL.
    • The go_to() method will simulate the URL, similar to the WordPress Core test suite, by modifying the global WP_Query to match the URL passed.
  • Trait contains a public run_for_urls( array $urls, callable $callback )
    • The method will loop over each of the URLs passed and simulate the URL using the go_to() method.
    • Then it will call the $callback function passed for each of the URLs passing an instance of the Check_Result class and the URL as parameters.

Tests Coverage

  • Enqueued_Scripts_Size_Check class is expected to extend Runtime_Check class
  • Enqueued_Scripts_Size_Check class is expected to be instance of Runtime_Check class
  • run method expected to append the audit results to $check_result
@mehulkaklotar mehulkaklotar moved this to Backlog in Plugin Check Nov 23, 2022
@mehulkaklotar mehulkaklotar added [Type] Enhancement A suggestion for improvement of an existing feature Checks Audit/test of the particular part of the plugin Infrastructure Issues for the overall plugin infrastructure and removed Infrastructure Issues for the overall plugin infrastructure labels Nov 23, 2022
@FlicHollis FlicHollis moved this from Backlog to Definition in Plugin Check Nov 29, 2022
@eclarke1 eclarke1 moved this from Definition to Backlog in Plugin Check Dec 7, 2022
@mehulkaklotar mehulkaklotar moved this from Backlog to Definition in Plugin Check Dec 9, 2022
@mehulkaklotar mehulkaklotar self-assigned this Dec 9, 2022
@mehulkaklotar mehulkaklotar moved this from Definition to AC Review in Plugin Check Dec 9, 2022
@mehulkaklotar mehulkaklotar removed their assignment Dec 9, 2022
@eclarke1 eclarke1 added this to the Sprint 3 milestone Jan 12, 2023
@felixarntz
Copy link
Member

@jjgrainger The ACs here will need a bit of iteration since they are old and not based on our implementation of Abstract_Runtime_Check.

However, let's take a step back here first: Reviewing our Abstract_Runtime_Check again, I realize we have not considered so far the reusable preparations that we will need (see https://docs.google.com/document/d/1Mge9i2_EMH7IEJD37vWXX9Uzyaz-p9awD1pI4AQ76Rg/edit?resourcekey=0-y7fxvr6cmxv_QW5xPylGcg#heading=h.j9uqeg4psica):

  • We have a place to run once-off check-specific preparations (the prepare method of any individual check class).
  • We (will) have a way to run universal preparations (see our conversation in Create Checks class #12).
  • But we do not have a way to run reusable preparations.

I think we need a way for check classes to express that they need certain reusable preparations. Maybe a new interface With_Reusable_Preparations that just has a static method like get_required_preparations() that returns an array of preparation class names? By having the method static, it allows the code to be called without instantiating the check class, which shouldn't be needed to get this information. With this approach we would later be able to implement the smart orchestration of reusable checks, as described in https://docs.google.com/document/d/1Mge9i2_EMH7IEJD37vWXX9Uzyaz-p9awD1pI4AQ76Rg/edit?resourcekey=0-y7fxvr6cmxv_QW5xPylGcg#heading=h.d8g2nxbaaltl.

WDYT? We will need a few more issues about that, that should probably take precedence over the first runtime checks #17 and #19.

@jjgrainger jjgrainger assigned jjgrainger and unassigned felixarntz Jan 19, 2023
@jjgrainger
Copy link
Contributor

Thanks @felixarntz

I've created a new issue #59 to implement the interface for shared preparations. I'll update these AC's next week when I'm back.

@vishalkakadiya vishalkakadiya self-assigned this Jan 23, 2023
@eclarke1 eclarke1 modified the milestones: Sprint 3, Sprint 4 Jan 24, 2023
@vishalkakadiya vishalkakadiya moved this from AC Review to In Progress in Plugin Check Jan 26, 2023
@vishalkakadiya vishalkakadiya self-assigned this Jan 26, 2023
@jjgrainger
Copy link
Contributor

Hi @felixarntz I have updated the AC's and are ready for you to review. Thanks!

@felixarntz
Copy link
Member

@jjgrainger ACs mostly look good, however a few things are missing or need to be clarified:

  • There is no reference that the Enqueued_Scripts_Size_Check::run() method should use the trait's run_for_urls() method. It's critical that basically the logic in that method that you have outlined needs to run for each of these URLs. We also need to then ensure that the threshold applies per URL (e.g. it could be that in one URL the script size exceeds the threshold while in another URL it is fine).
  • The check is missing that it needs to actually create the posts it goes through. Probably needs to rely on Create Demo_Posts_Creation_Preparation #40 and use that as a shared preparation. You mention "all publicly viewable post types" in get_urls(), but we also need to include in the ACs that those URLs need to be created first.

@felixarntz felixarntz assigned jjgrainger and unassigned felixarntz Feb 3, 2023
@jjgrainger jjgrainger modified the milestones: Sprint 4, Sprint 5 Feb 6, 2023
@jjgrainger
Copy link
Contributor

Thanks @felixarntz

I will update the AC's and dependencies. As this is dependent on #40 I have moved this to Sprint 5 where both issues will be worked on. cc @eclarke1

@jjgrainger jjgrainger moved this from AC Review to Definition in Plugin Check Feb 7, 2023
@jjgrainger jjgrainger moved this from Definition to AC Review in Plugin Check Feb 7, 2023
@jjgrainger jjgrainger assigned felixarntz and unassigned jjgrainger Feb 7, 2023
@felixarntz
Copy link
Member

@jjgrainger ACs pretty much LGTM! One follow up item to consider though:

If the script is part of the plugin, calculate the size of the script including any additional inline scripts.

We should probably also consider dependencies of the script. For example, if a plugin enqueues a tiny script which depends on jquery, react, and lodash to be loaded, it doesn't matter that the plugin script is tiny; it would still be a performance concern.

Maybe the dependency sizes could be stored in a separate variable so that the warning messaging can differ (e.g. "Your enqueued scripts use dependencies with a size of x" rather than "Your enqueued scripts have a size of x").

Since this is an additional effort and this issue is already quite substantial, we could also do this in a follow up issue though. WDYT? If you agree, please create that follow up issue, in that case the ACs here are good as is.

@felixarntz felixarntz assigned jjgrainger and unassigned felixarntz Feb 7, 2023
@jjgrainger
Copy link
Contributor

Thanks @felixarntz

I agree and have created a new issue for the follow up work - #74

@jjgrainger jjgrainger moved this from AC Review to To Do in Plugin Check Feb 8, 2023
@vishalkakadiya
Copy link

@jjgrainger I'm assigning it to myself and starting on it as I'm waiting for the final review on the Admin Tools page ACs. Thanks! 🙂

@vishalkakadiya vishalkakadiya moved this from To Do to In Progress in Plugin Check Feb 10, 2023
@jjgrainger jjgrainger moved this from In Progress to Code Review in Plugin Check Feb 22, 2023
@jjgrainger jjgrainger moved this from Code Review to In Progress in Plugin Check Feb 22, 2023
@jjgrainger jjgrainger moved this from In Progress to Code Review in Plugin Check Feb 24, 2023
@github-project-automation github-project-automation bot moved this from Code Review to Done in Plugin Check Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Checks Audit/test of the particular part of the plugin [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants