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

Avoid unnecessary side effects from client initialization and asset loading #980

Closed
felixarntz opened this issue Dec 13, 2019 · 2 comments
Closed
Labels
P0 High priority Type: Bug Something isn't working
Milestone

Comments

@felixarntz
Copy link
Member

felixarntz commented Dec 13, 2019

Bug Description

There are a couple of issues with client initialization and asset loading that may cause potential performance issues:

  • The client may attempt to refresh the access token on initialization which can be detrimental and sometimes unnecessary because initializing the client doesn't mean it will be used to actually trigger a request.
  • At the same time, we should avoid initializing the client when it is not actually needed. Currently we sometimes do that without care - if we just want to know whether there is an access token, that can happen plainly with an option lookup. In some cases, this unnecessary initialization can even happen in the frontend without someone being logged-in.
  • Partially related to that are some issues how we are loading assets. Particularly, the googlesitekit-admin.js, which is (rightfully) loaded on every admin screen, causes all the JS inline data to be generated which is fairly expensive but not actually used by that script at all. The script itself is okay to be loaded everywhere because it has minimal dependencies and actually is relevant, wherever the user currently is in the backend.

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

Acceptance criteria

  • Initializing the Google_Client should never trigger an API request in itself.
  • The Google_Client should only be initialized when we actually need it for an API request.
  • The inline JS data from PHP should only be generated when it is actually needed by one of the loaded scripts.

Implementation Brief

  • We should remove the check for whether the access token is expired in OAuth_Client::get_client(). Instead, we should fully rely on the on-the-fly refreshment of an expired access token that the Google_Client takes care of.
    • For the Google_Proxy_Client, we can add support for a token_exception_callback so that the plugin and user is still informed about any failing refresh attempts.
  • Wherever the OAuth_Client::get_client() is called just to get values that are actually from the DB, this should be avoided.
  • In the Module base class, when executing a single data request, only initialize the client if that data request is actually for a service (i.e. requires an API request that requires the client), avoiding further unnecessary initializations that could happen on regular page requests, even in the frontend.
  • Regarding Assets:
    • Move the JS inline data logic to sitekit-commons, which is the lowest-level script that relies on that data. Every other scripts that also needs that data relies on sitekit-commons anyway.
    • Make sure the googlesitekit-admin only has the dependencies it actually needs, which is wp-i18n, nothing more.
    • Optimize how inline JS data and other script- or stylesheet-related data is generated, so that it only happens whenever a script is actually enqueued (explicitly or as dependency). For that purpose, we can introduce a new before_print callback that is executed just-in-time whenever a script/stylesheet will actually be used in the current request.
  • Merge Avoid unnecessary client initialization, side effects and JS inline data generation #981

Changelog entry

  • Optimize initialization of the Google API client and minimize JavaScript assets and inline data being loaded in regular requests.
@felixarntz felixarntz added Type: Bug Something isn't working P0 High priority Size: S labels Dec 13, 2019
@felixarntz felixarntz added this to the 1.1.4 milestone Dec 13, 2019
@felixarntz felixarntz self-assigned this Dec 13, 2019
@felixarntz felixarntz assigned aaemnnosttv and ThierryA and unassigned felixarntz Dec 13, 2019
@aaemnnosttv
Copy link
Collaborator

IB ✅

@felixarntz felixarntz assigned felixarntz and unassigned felixarntz and ThierryA Dec 16, 2019
@felixarntz felixarntz modified the milestones: 1.1.4, 1.1.5 Dec 16, 2019
@felixarntz felixarntz assigned aaemnnosttv and unassigned felixarntz Dec 16, 2019
@felixarntz felixarntz removed their assignment Jan 5, 2020
@aaemnnosttv aaemnnosttv assigned felixarntz and unassigned aaemnnosttv Jan 6, 2020
@felixarntz felixarntz removed their assignment Jan 6, 2020
@cole10up
Copy link

cole10up commented Jan 8, 2020

Ran a set of regression around auth with the proxy paying attention to token expirations. All my tests passed.

Passed QA ✅

@ThierryA ThierryA modified the milestones: 1.2.0, Sprint 14 Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants