This issue was supposed to be part of:
#6432

But it's not a requirement for initial 3.16.

CC @MathieuLamiot

from wp-rocket.

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, is_cached, in rocket_data_lcp. This parameter could be used in the beacon itself to bailout before sending the request to the above_the_fold table. But, it turns out this approach has a flaw. If it's the first visit, the page won't be cached, so is_cached would be false, which is good. But, it would also cache the page with is_cached set to false. So, subsequent visits would always bail out. Plus, I realized I hadn't considered all possible file name combinations in the cache folders, which makes things even more complicated.

After chatting with @Tabrisrp, another idea was to do a DB request on the cache table, which can also be filtered by is_mobile. While this sounds good on paper, it doesn't really solve the problem. The main goal is to avoid unnecessary DB requests, but this new solution just swaps one DB request for another. Whether we query the cache table or the above_the_fold table, it ultimately adds another request. Also, it can even add a request, which I guess we don't want, in the following scenario:
When we load the page, we would check cache table, if it is cached, we will check above_the_fold for data, so it's even adding another request.

So, I'm a bit stumped on how to approach this issue. If anyone has an idea, please feel free.
@piotrbak @MathieuLamiot

from wp-rocket.

Indeed, checking the DB to know if the page is cached won't help here to reduce processing.
What I had in mind was to do the same way WPR Inspector does it here

It seems pages served from the cache have this at the end in the HTML:
Debug: cached@1714719972 -->

from wp-rocket.

This is only valid if the page gets cached with this in the wp-config:
define( 'WP_ROCKET_DEBUG', true );

EDIT: Maybe I was wrong, did a test that works without that constant. Let me check the code.

from wp-rocket.

Scope a solution

First of all, we need to detect when the page is cached or not.
To do that, within our class in lcp-beacon.js we need to add a new function:

	_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 Debug: cache which is added to the end of the signature when the page is cached.

Once done, we need to modify the function _isValidPreconditions() to add the check if the page is cached before sending a DB request through the AJAX call:

		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 _isGeneratedBefore() is currently always returning undefined we need to modify it to fix it:

	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

from wp-rocket.

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 Debug?

from wp-rocket.

@MathieuLamiot @piotrbak What will happen with the white label used? https://docs.wp-rocket.me/article/7-enabling-white-label

from wp-rocket.

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.

from wp-rocket.

Going to create a github issue for this now

from wp-rocket.

I created the issue here: #6637
If u have any more input plz add it there.

For the whitelabel point:

When the constant WP_ROCKET_WHITE_LABEL_FOOTPRINT is there we still add the Debug: cached here and we search for it in the new beacon here So we are fine here

from wp-rocket.

Thanks boss!

from wp-rocket.

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
Moving this back to In progress.

from wp-rocket.