google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.25k stars 291 forks source link

Requests made for module settings on dashboard #9178

Open aaemnnosttv opened 3 months ago

aaemnnosttv commented 3 months ago

Bug Description

Module settings almost never need to be requested via the API for reading since the responses are preloaded for all active modules, however the TTL of the preloaded data is short-lived.

On the SK dashboard, it can be observed that settings are being requested for both Search Console & Analytics (when not active).

Steps to reproduce

  1. Do minimal setup of SK with no additional modules
  2. Observe settings requests when loading the dashboard

Additional Context


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation Brief

Test Coverage

QA Brief

Changelog entry

eugene-manuilov commented 2 months ago

IB ✔️

aaemnnosttv commented 2 months ago

@zutigrm it seems like you're suggesting functionality which is already implemented as module settings are already preloaded automatically for modules that implement the interface in https://github.com/google/site-kit-wp/blob/b6c7c59c9bece36c8b0138d2e3d4f05a2f09479a/includes/Core/Modules/REST_Modules_Controller.php#L68-L82

The reason for the Analytics settings request is due to the use of a settings-backed selector in a component which is rendered independently of the GA module activation (see notification mentioned above) so the settings are not preloaded and the proposed solution would have the same limitation as only active modules have their register method called.

Also the IB does not address the Search Console settings request which is mentioned above. I think we may need to increase the TTL for preloaded settings.

zutigrm commented 1 month ago

@aaemnnosttv I have updated the IB per our last discussion we had around this issue on one of the 1:1 calls

aaemnnosttv commented 3 days ago
  • Adjust the timeout to be calculated more dynamically, by using the length of Object.keys( preloadedData ) multiplied by 150 for example

Thanks @zutigrm – this is an interesting idea but I'm not sure the issue is correlated to how much data is being preloaded, but how many requests are happening on the dashboard which delays when the requests are triggered and processed. I think ideally, we'd use some kind of a signal that the current view had completely loaded rather than a timeout, but can't really think of a reliable way to do that. For now, the simplest solution is probably to just bump the timeout (preloaded data TTL) to 2 or 3 sec instead of 1. In my testing, doubling it to 2 worked fine to fix the SC settings request (only thing left to address, correct?) but LMK if you have any other ideas.