Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Closes #6764: Refactor lcp beacon script #6789

Open
wants to merge 4 commits into
base: 3.17-atf-refactor
Choose a base branch
from
Open

Closes #6764: Refactor lcp beacon script #6789

wants to merge 4 commits into from

Conversation

Miraeld
Copy link
Contributor

@Miraeld Miraeld commented Jul 17, 2024

Change the structure of the beacon script to include a new class RocketBeacon which will contains more generic functions that could be shared with other features.

Description

Fixes #6764

Documentation

User documentation

This shouldn't impact users.

Technical documentation

The beacon script has been renamed to wpr-beacon.js & has been refactored.
This refactor includes the addition of a new class named RocketBeacon which contains generic functions used later on by RocketLcpBeacon class.
It will allow us to add more feature easily to avoid duplication.
In order to test this PR, wp-media/wp-rocket-e2e#124 needs to be merged into e2e.

Type of change

  • Enhancement (non-breaking change which improves an existing functionality).

Checklists

Feature validation

  • I validated all the Acceptance Criteria. If possible, provide sreenshots or videos.
  • I triggered all changed lines of code at least once without new errors/warnings/notices.
  • I implemented built-in tests to cover the new/changed code.

Documentation

  • I prepared the user documentation for the feature/enhancement and shared it in the PR or the GitHub issue.
  • The user documentation covers new/changed entry points (endpoints, WP hooks, configuration files, ...).
  • I prepared the technical documentation if needed, and shared it in the PR or the GitHub issue.

Code style

  • I wrote self-explanatory code about what it does.
  • I wrote comments to explain why it does it.
  • I named variables and functions explicitely.
  • I protected entry points against unexpected inputs.
  • I did not introduce unecessary complexity.
  • I listed the introduced external dependencies explicitely on the PR.
  • I validated the repo-specific guidelines from CONTRIBUTING.md.

Observability

  • I handled errors when needed.
  •  I wrote user-facing messages that are understandable and provide actionable feedbacks.
  • I prepared ways to observe the implemented system (logs, data, etc.).

Risks

  •  I explicitely mentioned performance risks in the PR.
  • I explicitely mentioned security risks in the PR.

@Miraeld
Closes #6764: Refactor lcp beacon script
9497bb3 
Change the structure of the beacon script to include a new class `RocketBeacon` which will contains more generic functions that could be shared with other features.
@Miraeld Miraeld added type: enhancement Improvements that slightly enhance existing functionality and are fast to implement lcp labels Jul 17, 2024
@codacy-production Codacy Production
Copy link

codacy-production bot commented Jul 17, 2024 •
edited
Loading

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
Report missing for 73a7e8d1 ✅ 100.00% (target: 50.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (73a7e8d) Report Missing Report Missing Report Missing
Head commit (3d80744) 37374 14549 38.93%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#6789) 4 4 100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

🚀 Don’t miss a bit, follow what’s new on Codacy.

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

Footnotes

  1. Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩

@piotrbak piotrbak added module: OCI Optimize Critical Images and removed lcp labels Jul 18, 2024
jeawhanlee
jeawhanlee reviewed Jul 18, 2024
View reviewed changes
assets/js/wpr-beacon.js Outdated Show resolved Hide resolved
assets/js/wpr-beacon.js Outdated Show resolved Hide resolved
@Tabrisrp
Copy link
Contributor

Should this PR be done against develop? Shouldn't it be against the refactor branch?

@Miraeld Miraeld changed the base branch from develop to 3.17-atf-refactor July 18, 2024 23:31
@Miraeld
Copy link
Contributor Author

Miraeld commented Jul 19, 2024

@jeawhanlee I've updated action names and everything related to that.

@Tabrisrp I've changed the based branch.

Tabrisrp
Tabrisrp requested changes Jul 19, 2024
View reviewed changes
inc/Engine/Media/AboveTheFold/Frontend/Controller.php
@@ -412,15 +412,15 @@ public function inject_beacon( $html, $url, $is_mobile ): string {
*
* @param int $delay The delay in milliseconds. Default is 500.
*/
$delay = apply_filters( 'rocket_lcp_delay', $default_delay );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we change the filter name, we should add an apply_filters_deprecated call too for backward compatibility

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeawhanlee jeawhanlee jeawhanlee left review comments

@Tabrisrp Tabrisrp Tabrisrp requested changes

Assignees

@Miraeld Miraeld

Labels
module: OCI Optimize Critical Images type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3.17 - Refactor beacon script
4 participants
@Miraeld @Tabrisrp @jeawhanlee @piotrbak