Problem/Motivation

Unless instructed otherwise, Drupal core's aggregation system builds aggregates using all of the JS files attached to a page; this is works fine when doing full page loads because a given page only has one instance of each JS file aggregated, but this leads to JavaScript errors and broken interactions when navigating around a site using something like Turbo that adds new <script/> elements it finds in responses. With JS aggregation disabled, Turbo correctly determines which <script/> are already attached and does not attach duplicates; however, when JS aggregation is enabled, Turbo has no way to know that an aggregate with a different file name contains some of the same JavaScript code already attached and executed on the current page, nor is there a reliable way that it could figure this out. Other projects seem to have run into this as well; see #3411010: Document that aggregation of JavaScript files can cause redeclaration.

Steps to reproduce

Enable JavaScript aggregation. Navigate around and start seeing the console debug messages multiply, along with various errors when behaviours are double, triple, etc. attached:

RefreshLess: redirected /wiki/Special:Random → /wiki/2049/10/03/Adira js_hhgE22TbJoFEc0p2gGcm5NrkLElXp2JKD5t-ppI6IeU.js:1859:2492
RefreshLess: redirected /wiki/Special:Random → /wiki/2049/10/03/Adira js_91B8e_fr_qMZjN308me2y5aKFcx82T_6cWN5klDnuFc.js:1859:2492
RefreshLess: redirected /wiki/Special:Random → /wiki/2049/10/03/Adira js_hhgE22TbJoFEc0p2gGcm5NrkLElXp2JKD5t-ppI6IeU.js:1859:2492
RefreshLess: detaching behaviours js_hhgE22TbJoFEc0p2gGcm5NrkLElXp2JKD5t-ppI6IeU.js:1861:1450
RefreshLess: detaching behaviours js_91B8e_fr_qMZjN308me2y5aKFcx82T_6cWN5klDnuFc.js:1861:1450
RefreshLess: detaching behaviours js_hhgE22TbJoFEc0p2gGcm5NrkLElXp2JKD5t-ppI6IeU.js:1861:1450
RefreshLess: detaching behaviours js_hhgE22TbJoFEc0p2gGcm5NrkLElXp2JKD5t-ppI6IeU.js:1861:1450
RefreshLess: drupalSettings has been updated js_hhgE22TbJoFEc0p2gGcm5NrkLElXp2JKD5t-ppI6IeU.js:1859:631
RefreshLess: drupalSettings has been updated js_91B8e_fr_qMZjN308me2y5aKFcx82T_6cWN5klDnuFc.js:1859:631
RefreshLess: drupalSettings has been updated js_hhgE22TbJoFEc0p2gGcm5NrkLElXp2JKD5t-ppI6IeU.js:1859:631
RefreshLess: attaching behaviours js_hhgE22TbJoFEc0p2gGcm5NrkLElXp2JKD5t-ppI6IeU.js:1861:1286
RefreshLess: detaching behaviours js_hhgE22TbJoFEc0p2gGcm5NrkLElXp2JKD5t-ppI6IeU.js:1861:1450
RefreshLess: attaching behaviours js_hhgE22TbJoFEc0p2gGcm5NrkLElXp2JKD5t-ppI6IeU.js:1861:1286
RefreshLess: attaching behaviours js_hhgE22TbJoFEc0p2gGcm5NrkLElXp2JKD5t-ppI6IeU.js:1861:1286
RefreshLess: attaching behaviours js_91B8e_fr_qMZjN308me2y5aKFcx82T_6cWN5klDnuFc.js:1861:1286

Proposed resolution

Core has built-in support for the Ajax page state, which includes which libraries are already attached to a page so that Ajax (and similar) responses don't cause multiple execution of JavaScript already on a page. Couple that with Drupal 10.1's improved aggregation system that makes it easier to build aggregates with a given set of libraries included and excluded, and we have the starting point for additive aggregation.

Short term fixes were attempted, such as preprocess: false on all of our JavaScript and wrapping all our classes in once(), but these only fixed the symptoms in our own libraries, not those of core, other modules, or themes.

Relevant may be core issues #3227837: Optimize aggregation grouping and #3232810: [PP-1] Allow setting aggregation groups for js files in library definitions; while not directly fixing our issue, #3224261: [PP-1] Front-end performance: concatenate CKEditor 5 JS assets for every unique configuration has some interesting and potentially useful ideas.

Remaining tasks

Now that the prototype additive aggregation seems to be working, we have the following still left to polish and refine it:

  • ✅ Ensure that changing aggregation settings causes Turbo to perform a reload, i.e. when:
    1. CSS aggregation is enabled or disabled.
    2. JS aggregation is enabled or disabled.
    3. The CSS/JS query string changes.


    These can output on a <meta> element with a data-turbo-track="reload" attribute.

  • ⏳ Add a service to determine if the current request is a Turbo request; add a cache context that uses it and make sure we vary by it; have our decorated asset resolver, additive libraries, middleware, and so on use it to only act on Turbo requests and not other types (full page loads, Ajax, etc.).
  • ✅ The AdditiveLibraries middleware currently responds before core's AjaxPageState middleware, but all that the latter does is decompress the libraries query parameter, which means that AdditiveLibraries currently has to decompress, alter the libraries, and then recompress it again only for AjaxPageState to decompress it again; it may be best to have our AdditiveLibraries run after AjaxPageState to avoid this unnecessary work.
  • ✅ Bug: if switching to a tab that was unloaded in Firefox, it sometimes results in a page that doesn't have all the styles and JavaScript, likely to the browser having cached only the additive version; this may need a reload if we detect this; doesn't seem to be a problem in Chromium. May also be fixable once we re-enable Turbo caching.
  • ✅ Add a page cache request policy to prevent caching Turbo requests so that additive JS aggregation varies correctly with the page cache module installed.

Follow up tasks

  • ❌ Tests for additive aggregation; see #3401146: Turbo: Automated tests
  • ❌ Add data-turbo-track="reload" to this module's JavaScript. This will not be compatible with aggregation unless core is patched or begins to support it, so will need figure out how to handle that.
  • ❌ Simplify and merge the duplicate logic between the middleware and our decorated asset resolver into a central class or service. An custom attachments processor might be the best place; see the HTMX module's HtmxResponseAttachmentsProcessor.
  • ❌ Turbo itself is currently not minified/aggregated as that seems to cause it to re-evaluate sometimes, resulting in more than one instance of Turbo on the page; long term, having Turbo minified would be ideal, even if in its own separate aggregation group. 4xx responses might be one of the triggers for this, though other sources might also be. The solution to this may require building Turbo from source ourselves.

User interface changes

Stuff less busted.

API changes

None?

Data model changes

None.

References

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

Ambient.Impact created an issue. See original summary.

ambient.impact’s picture

Issue summary: View changes
ambient.impact’s picture

Issue summary: View changes

Added more detail, links to relevant Drupal core and Turbo code, and an example of one hack that seems to work around the issue (though not fixing the underlying problem).

ambient.impact’s picture

  • Ambient.Impact committed 9ed32e82 on 2.x
    Issue #3414538: refreshless_turbo/js/refreshless.js data-turbo-track="...

  • Ambient.Impact committed 95134ffb on 2.x
    Issue #3414538: preprocess: false on refreshless_turbo/js/refreshless.js
    

  • Ambient.Impact committed 8ce2c37d on 2.x
    refreshless_turbo/js/ajax.js: Merge .use-ajax into once():
    
    This may...

  • Ambient.Impact committed 21ff6396 on 2.x
    Issues #3414538 & #3399243: Fix drupalSettingsLoader w/ JS aggregation.
    
ambient.impact’s picture

Status: Active » Postponed

Setting to postponed for now; we can clearly state that a known limitation of this Turbo implementation is that JavaScript aggregation should be off or weird things will happen which should be fine for a dev or alpha release; when we start getting to beta, we'll need to implement the longer term proposed resolution in some way.

catch’s picture

One possibility would be to track $ajax_page_state in session, and then maybe in a middleware, take what's in $_SESSION and fake the query parameter (probably merging with whatever's already in there just in case), then Drupal won't load any of those already-loaded libraries.

ambient.impact’s picture

That may work, yeah, but I'm wondering if forcing a session for all users - including anonymous users - is going to have a noticeable performance hit. By default, Symfony doesn't start a session until there's something in the session to track (like in this case the Ajax page state) for performance reasons. That said, we do force a session for all users on Omnipedia to track the last few wiki pages a user has visited so that the random page controller knows not to pick something you've just seem, and that seems to perform well enough. If we do start sessions for all users, it would be a good idea to require the core page_cache module is uninstalled, because it'll prevent a session being started if a page is already cached - it took me far longer than I would have liked to figure out it was why our random page controller would sometimes redirect to the same page over and over for anonymous users.

catch’s picture

Might be possible to set it in a cookie instead as well, could still be read in a middleware.

ambient.impact’s picture

I haven't messed around with any middleware as of yet so I'm not experienced with that but I like the idea. If anyone wants to create a basic prototype I'd be happy to test it out. Sorry about the slow replies; I'm busy with a bunch of stuff at the moment but checking in every so often.

ambient.impact’s picture

So I tried to implement a solution based on @catch's idea (see the issue fork) and it sort of works but not quite. It doesn't yet have a cache context to vary by and it breaks Turbo since the Turbo JavaScript isn't found on the page so Turbo does a full page load, and I feel like piggybacking on ajaxPageState might not be worth the potential headaches down the road if that changes. That said, the overall idea seems good, but it needs a bunch of work to make it functional.

To do list/ideas

  • Implement a cache context that varies based on a hash or compressed URL query string because we'll likely need to add that to the page #cache.
  • Port over some of the 1.x refreshless_page_state logic and don't spoof or use ajaxPageState.
  • Also don't forget RefreshlessRenderer::validatePreconditions() for security from the 1.x branch.
  • Turbo and RefreshLess stuff needs to be handled separately and excluded from being removed on subsequent pages because doing so causes Turbo to relinquish control and let a full page load occur. We may have to decorate another one of the core asset services to group the RefreshLess and Turbo stuff into its own aggregate since setting it all to preprocess: false isn't great.
  • Implement the PHP logic somewhere to exclude the already loaded libraries somewhere; our decorated AssetResolver would make the most sense since it's central to the whole process.

Core stuff to use as reference

\Drupal\big_pipe\Render\BigPipe::sendContent()
Example of dealing with cumulative asset libraries.
\Drupal\Core\Asset\AttachedAssetsInterface::setAlreadyLoadedLibraries()
Can be used to mark libraries as already loaded on a page and thus to not be loaded again.
\Drupal\Core\Asset\AssetResolver::getLibrariesToLoad()
Uses the above to remove already loaded libraries. We don't need to invoke this ourselves but just need to do the library marking as loaded before we hand off getCssAssets() and getJsAssets() to the decorated service.
\system_js_settings_alter()
Example of how ajaxPageState libraries are built.
\Drupal\Core\StackMiddleware\AjaxPageState::parseAjaxPageState()
Decompresses compressed libraries.
ambient.impact’s picture

Status: Postponed » Needs work

Alright, so I think I have it mostly working. It should now be aggregating and
attaching only what's not already on the page, with the following caveats/problems yet to be resolved:

  • Our module JavaScript is set to preprocess: false for now because it doesn't seem possible to group ourselves into a separate aggregate; see #3232810: [PP-1] Allow setting aggregation groups for js files in library definitions. Ideally, we would be grouped into one aggregate that contains just the refreshless_turbo/refreshless and refreshless_turbo/turbo libraries so that we could take advantage of core's minification and gzipping.
  • We have to special case the refreshless_turbo/refreshless and refreshless_turbo/turbo libraries to ensure Turbo is always attached even with the additive libraries logic, but never attached or aggregated more than once because bad things happen when Turbo loads and evaluates itself a second time.
  • There's some duplicate logic between the middleware and our decorated asset resolver that I'm not happy with but I couldn't get it working well enough without it. I'll probably rewrite some of it into an attachments processor (see the HTMX module's HtmxResponseAttachmentsProcessor) to hopefully remove the duplication.
  • This re-introduces the same CSS specificity symptoms as seen in #3399314: Turbo: CSS files merged into the <head> always appended instead of preserving full page load order, causing CSS specificity issues but now when aggregation is enabled as well: the cause now seems to be that the base theme's (Claro) CSS for dropbuttons and vertical tabs is additively aggregated (since they're defined in separate libraries and not on every page) while Gin compiles dropbuttons into its big gin.css that gets attached to every page and uses a fixed weight value on its tabs.css library definition rather than listing Claro's as a dependency. The best course of action may be to rework the additive aggregation so that it only applies to JavaScript and not libraries as a whole, leaving CSS to aggregate as it did before, non-additively; this may be doable via our decorated asset resolver in the getCssAssets() method by checking if the current request is a Turbo request and then temporarily removing the already loaded libraries, calling $this->decorated->getCssAssets(), then restoring the already loaded libraries before returning the CSS assets.
  • It's unclear if this needs a cache context because this seems to be working with caching, but my gut is saying that we should have a cache context and set up a clear distinction between a full page load and a Turbo/additive load, because there's a bunch of stuff in the code that just YOLOs it without a care which might backfire in the future.

So it still needs work but some progress has been made. The hardest part of this is working without an easy way to visualize what libraries are being (additively) aggregated and attached to a page. Setting up some tests for that would help, but I would also want to implement console logging of what new libraries get added, much like the 8.x-1.x branch has, and may something a bit more visual would help as well.

ambient.impact’s picture

Title: Turbo: JS aggregation causes the RefreshLess JavaScript to evaluate more and more times on navigation » Turbo: Implement additive JavaScript aggregation to prevent multiple evaluation

Updating title as we now know what needed to be done and why.

ambient.impact’s picture

Issue summary: View changes

Heavy rewrite of issue summary reflecting current understanding of the problem and solution.

ambient.impact’s picture

Issue summary: View changes

Added remaining tasks and follow up tasks.

ambient.impact’s picture

Issue summary: View changes

Minor grammar fix.

ambient.impact’s picture

Issue summary: View changes
ambient.impact’s picture

Issue summary: View changes

Updated status of a few items.

ambient.impact’s picture

Issue summary: View changes

Reordered "Remaining tasks" and "Follow up tasks", and moved some stuff between them where they make more sense.

ambient.impact’s picture

Issue summary: View changes
Related issues: +#3401146: Turbo: Automated tests

Link to #3401146: Turbo: Automated tests for test details.

ambient.impact’s picture

Issue summary: View changes

Added a few remaining tasks from my comments.

ambient.impact’s picture

Issue summary: View changes

Added References section with links to the HTMX module.

ambient.impact’s picture

Issue summary: View changes

Removed session item (not actually true) and added page cache request policy item.

  • ambient.impact committed 656a0a24 on 2.x
    Issue #3414538: Ajax page state theme_token output only if diff theme:...

  • ambient.impact committed 0591e0fc on 2.x
    Issue #3414538: Updated readme.md with current state of JS aggregation.
    

  • ambient.impact committed 48c15bac on 2.x
    Issue #3414538: Additive libraries middleware now runs after core Ajax...

  • ambient.impact committed c13e9fb0 on 2.x
    Issue #3414538: Additive libraries middleware now uses context service.
    

  • ambient.impact committed 21c73950 on 2.x
    Issue #3414538: Force reload if page restored in possible broken state.
    

  • ambient.impact committed 2b6227e1 on 2.x
    Issue #3414538: Add a service to detect Turbo requests + cache context.
    

  • ambient.impact committed cf9a47a0 on 2.x
    Issue #3414538: Add Turbo reload <meta> for aggregation config changes.
    

  • ambient.impact committed 1a519967 on 2.x
    Issue #3414538: Cleaned up and documented Javascript::outputPageState().
    

  • ambient.impact committed 55efc419 on 2.x
    Issue #3414538: Updated JS aggregation readme section now that it works.
    

  • ambient.impact committed aa0ab207 on 2.x
    Issue #3414538: Enabled aggregation for our own JS if core patched:
    
    See...

  • ambient.impact committed 8e8de278 on 2.x
    Issues #3414538 & #3399314: Additive JS no longer breaks library CSS.
    

  • ambient.impact committed 8483884d on 2.x
    Issue #3414538: Remove __Host- prefix from page state cookie for now.
    

  • ambient.impact committed 8098adf2 on 2.x
    Issue #3414538: Set our own and Turbo JS to preprocess: false for now.
    

  • ambient.impact committed 98e7736c on 2.x
    Issue #3414538: First attempt at additive JS aggregation; not working.
    
ambient.impact’s picture

Status: Needs work » Postponed
ambient.impact’s picture

I've created a new branch (3414538-what-if-attachments-processor) in the issue fork to try and implement a decorated attachments processor, but I can't yet get it to work. The main motivation here is to no longer rely on ajaxPageState because outputting it on every page results in certain generated URLs that have a query string (e.g. pager, admin/config compact toggle link, etc.) also including the Ajax page state in their URL, which in turn causes some weird behaviour with those links where Turbo doesn't kick in during history back/forward navigation, in addition to looking messy and ugly.

There's something I'm not understanding and not doing correctly with the library diffing, and without a clear indicator of what libraries are being added and when, it's hard to pin down. I think this would need some kind of debugging output when libraries are added to hopefully spot what's going wrong, but I'm tired and need to do other stuff right now.