-
Notifications
You must be signed in to change notification settings - Fork 210
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
Reduce the processing on not-cached page when checking the LCP/ATF data #6571
Comments
This issue was supposed to be part of: But it's not a requirement for initial 3.16. |
Hello hello, I get that we want to tackle this issue. I've been brainstorming some solutions and had a chat with @Tabrisrp. One idea was to check if the visited URL was cached here. The plan was to introduce a new parameter, After chatting with @Tabrisrp, another idea was to do a DB request on the So, I'm a bit stumped on how to approach this issue. If anyone has an idea, please feel free. |
Indeed, checking the DB to know if the page is cached won't help here to reduce processing. It seems pages served from the cache have this at the end in the HTML: |
This is only valid if the page gets cached with this in the EDIT: Maybe I was wrong, did a test that works without that constant. Let me check the code. |
Scope a solutionFirst of all, we need to detect when the page is cached or not. _isPageCached() {
let signature = '';
let status = false;
if (document.documentElement.nextSibling) {
signature = document.documentElement.nextSibling.data;
}
if ( signature && signature.includes( 'Debug: cached' ) ) {
return true;
}
return false;
} This function will check the signature from WP Rocket to see if there is the part Once done, we need to modify the function if ( this._isPageCached() ) {
if ( this._isGeneratedBefore() ) {
this._logMessage('Bailing out because data is already available');
return false;
} And finally, as I noticed while grooming that async _isGeneratedBefore() {
// AJAX call to check if there are any records for the current URL.
let lcp_data_response;
let data_check = new FormData();
data_check.append('action', 'rocket_check_lcp');
data_check.append('rocket_lcp_nonce', this.config.nonce);
data_check.append('url', this.config.url);
data_check.append('is_mobile', this.config.is_mobile);
await fetch(this.config.ajax_url, {
method: "POST",
credentials: 'same-origin',
body: data_check
})
.then(data => {
lcp_data_response = data
});
return lcp_data_response.success;
} Efforts:S |
Just a though crossing my mind reading this: If we start using the signature as a "feature", maybe it'd be good to change the wording to something else than |
@MathieuLamiot @piotrbak What will happen with the white label used? https://docs.wp-rocket.me/article/7-enabling-white-label |
We should handle it as well, as I mentioned here: #6610 (comment) But the PR lacks follow-up, we discussed opening a follow-up issue for the discussed enhancement (possibly including the white label handling) a few days ago already. @wp-media/engineering-plugin-team can you make sure to capture the leftover discussed in the PR to a dedicated GH issue so that the scope of this PR vs. Leftover is clear and we can close it. |
Going to create a github issue for this now |
Thanks boss! |
Async/Await issues: the fetch method is async so we can't use it in a sync process to check an HTTP response. Discussed here |
…the LCP/ATF data (#6610) Co-authored-by: Opeyemi Ibrahim <opeyemi.khadri@gmail.com> Co-authored-by: WordPressFan <ahmed@wp-media.me>
Is your feature request related to a problem? Please describe.
On not cached page we're checking if LCP/ATF data exists twice:
Describe the solution you'd like
Instead of checking it for the second time, we could bail-out if we're on not cached page. That's potentially an improvement in terms of resource usage.
Acceptance Criteria
The text was updated successfully, but these errors were encountered: