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.

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

As a DelayJS user, I want to rollback to previous DelayJS version to mitigate regressions #7349

Closed
MathieuLamiot opened this issue Mar 9, 2025 · 7 comments · Fixed by #7350
Assignees
Labels
effort: [XS] < 1 day of estimated development time
Milestone

Comments

@MathieuLamiot
Copy link
Contributor

Context

We recently released a complete rework of the DElay JS script with the v2.
While it fixes many issues, it has some regressions on other points, that are being tackled by the developers. However, in the meantime, we would like to offer an easy way for users to use a previous version of the script.

Dependencies

NA

Expected behavior

Provide a filter allowing to select which version of DelayJS should be used.
For now, only the current version and v1.2.6 must be provided.
Note that DelayJS is composed of 2 scripts: lazyload-script and elementor-animation.

Provide the filter name and how to use it to @piotrbak and the support team so that they can prepare a helper plugin.

Acceptance Criteria

  • Given that no hook is registered on the filter, the delayJS current version must be injected.
  • Given that a hook changes the filtered version to 1.2.6, the v1.2.6 of the scripts must be injected.
  • Given that a hook changed the filtered version to something else than '1.2.6', the current version must be injected.

Additional information

  • For the filter, use the apply_filter_typed library.
  • For now, we will only provide the current Delay JS (by default) and the v.1.2.6 (see here). The v1.2.6 was replaced with v2 in this PR
  • In the future, we will provide more versions of DelayJS, maybe with an automated export from the DelayJS repo to WP Rocket. For now, we want to make it manual and quick with just 1.2.6. But we must keep this in mind so that the filter we provide can allow more versions in the future. (for instance, filter a string being the script version, and fallback on the current version is the files are not found).
@MathieuLamiot
Copy link
Contributor Author

MathieuLamiot commented Mar 9, 2025

Grooming

Scope a solution
To implement this feature, we must:

  • Add v1.2.6 files (2 scripts) to WP Rocket repo, alongside the current versions.
  • Add a filter before injecting those scripts to maybe inject a versioned file.

Development steps:

  • Add a filter to pick the script version, empty string by default.
  • Look for lazyload-script-version_number.min.js & elementor-animation-version_number.js
    • if they exist, inject them.
    • else, inject the default lazyload-script.min.js & elementor-animation.js. Log a message to warn users the filter behavior has not been followed.
  • Manually add lazyload-script-1.2.6.min.js & elementor-animation-1.2.6.js to the repo

How will this be validated?

  • Manually check the ACs
  • Implement unit tests for the filter's behavior
  • Run Rocket-E2E (DelayJS part) for non-regression checks.

Can be peer-coded: No
Is a refactor needed in that part of the codebase? No
Effort: XS

@MathieuLamiot
Copy link
Contributor Author

cc @piotrbak Adding this to the service board to help with 3.18.3
Let me know if the issue description is not OK for you

@MathieuLamiot MathieuLamiot added this to the 3.18.3 milestone Mar 9, 2025
@MathieuLamiot MathieuLamiot added the effort: [XS] < 1 day of estimated development time label Mar 9, 2025
@CrochetFeve0251
Copy link
Contributor

Grooming

Scope a solution To implement this feature, we must:

  • Add v1.2.6 files (2 scripts) to WP Rocket repo, alongside the current versions.
  • Add a filter before injecting those scripts to maybe inject a versioned file.

Development steps:

  • Add a filter to pick the script version, empty string by default.

  • Look for lazyload-script-version_number.min.js & elementor-animation-version_number.js

    • if they exist, inject them.
    • else, inject the default lazyload-script.min.js & elementor-animation.js. Log a message to warn users the filter behavior has not been followed.
  • Manually add lazyload-script-1.2.6.min.js & elementor-animation-1.2.6.js to the repo

How will this be validated?

  • Manually check the ACs
  • Implement unit tests for the filter's behavior
  • Run Rocket-E2E (DelayJS part) for non-regression checks.

Can be peer-coded: No Is a refactor needed in that part of the codebase? No Effort: XS

@MathieuLamiot I would be fine with that solution but it works only with the verson 1.2.6 and not the previous ones.

Should we add also previous versions or is it fine because your AC states Given that a hook changed the filtered version to something else than '1.2.6', the current version must be injected. which means we need also to include 1.2.5 to right?

@CrochetFeve0251
Copy link
Contributor

CrochetFeve0251 commented Mar 9, 2025

@MathieuLamiot I added a PR for that code I just need to have someone to Run Rocket-E2E (DelayJS part) for non-regression checks.

Small question for the moment I didn't renammed the delay js script v2 with the version, I was wanting to know should it be done so we have all version on each script or can it have a side effect?

@CrochetFeve0251 CrochetFeve0251 self-assigned this Mar 9, 2025
@MathieuLamiot
Copy link
Contributor Author

Should we add also previous versions or is it fine because your AC states Given that a hook changed the filtered version to something else than '1.2.6', the current version must be injected. which means we need also to include 1.2.5 to right?

NO we should not add other versions than 1.2.6 for now. If the filter selects something else, we will serve the current (the latest release) version so the one already in the repo.

@MathieuLamiot
Copy link
Contributor Author

@CrochetFeve0251 For Rocket-E2E, please contact the QA team, or make this Ready for QA and specify E2E needs to run.

@MathieuLamiot
Copy link
Contributor Author

Small question for the moment I didn't renammed the delay js script v2 with the version, I was wanting to know should it be done so we have all version on each script or can it have a side effect?

I am not sure to understand the question. Do you mean you want to rename the "current version" of the script too? I would say it has no benefits to do so. Keeping the normal name allows to know it's the default/latest version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: [XS] < 1 day of estimated development time
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants