Problem/Motivation

From a CWV point of view, adding the additional JS and CSS when there are no active alerts and auto-refresh is disabled is sub-optimal

Steps to reproduce

Proposed resolution

Don't attach the library when there are no active alerts and auto-refresh is disabled

Remaining tasks

User interface changes

API changes

Data model changes

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

larowlan created an issue. See original summary.

larowlan’s picture

Status: Active » Needs review

Was a bit gnarly on the caching front, also fixed what is likely a cache bug with turning the 'show on admin' on and off.

danielveza’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me! Reducing JS loaded & better cachability is a big win.

chrissnyder made their first commit to this issue’s fork.

  • chrissnyder committed 7ce6ea5 on 2.x authored by larowlan
    Issue #3261893 by larowlan: Don't attach the library etc if there are no...
chrissnyder’s picture

Merged and marked fixed! Thanks for the work and testing!

chrissnyder’s picture

Status: Reviewed & tested by the community » Fixed
chrissnyder’s picture

Status: Fixed » Needs work

I am having second thoughts on this. Mainly because this invalidates full-page caching causing the cacheability of pages to be independent of if there are alerts or not. One of the original goals of this module was to not have alerts impact page caching.

An example demonstrating my concern and why this is important:

A university, school, or government agency needs to publish a very important time-sensitive alert sitewide at a time when traffic to the site is abnormally high. This may be an alert regarding public safety. With this change, adding a sitewide alert would cause the page cache of all the pages on the site to be invalidated (due to the cache tag) resulting in excessive stress on the site as it needs to rebuild all of the pages at a time when traffic is high and the information is important.

One of the main goals of loading the alerts as a separate request was to allow pages to be fully cached independently of active alerts.

  • chrissnyder committed abf44b9 on 2.x
    Revert "Issue #3261893 by larowlan: Don't attach the library etc if...
larowlan’s picture

Ok - we'll just keep applying the patch to improve our core web vital scores - thanks for considering it

chrissnyder’s picture

Larowlan,

I did incorporate some of the changes from your work in #3264255: Ensure library attachment is dependent on settings. Thank you for those changes!.

chrissnyder’s picture

Status: Needs work » Closed (works as designed)
larowlan’s picture

No worries, we've copied the tests from here into our project and are patching for now.
We'll likely use hook_module_implements_alter to stub out the sitewide_alert_page_top and put it in a block instead
Thanks for this module