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:1286Proposed 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:
- CSS aggregation is enabled or disabled.
- JS aggregation is enabled or disabled.
- The CSS/JS query string changes.
These can output on a<meta>element with adata-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
AdditiveLibrariesmiddleware currently responds before core'sAjaxPageStatemiddleware, but all that the latter does is decompress the libraries query parameter, which means thatAdditiveLibrariescurrently has to decompress, alter the libraries, and then recompress it again only forAjaxPageStateto decompress it again; it may be best to have ourAdditiveLibrariesrun afterAjaxPageStateto 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
Issue fork refreshless-3414538
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
Comment #2
ambient.impactComment #3
ambient.impactAdded 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).
Comment #4
ambient.impactComment #10
ambient.impactComment #11
ambient.impactSetting 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.
Comment #12
catchOne 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.
Comment #13
ambient.impactThat 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_cachemodule 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.Comment #14
catchMight be possible to set it in a cookie instead as well, could still be read in a middleware.
Comment #15
ambient.impactI 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.
Comment #16
ambient.impactSo 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
ajaxPageStatemight 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
#cache.refreshless_page_statelogic and don't spoof or useajaxPageState.RefreshlessRenderer::validatePreconditions()for security from the 1.x branch.preprocess: falseisn't great.AssetResolverwould make the most sense since it's central to the whole process.Core stuff to use as reference
\Drupal\big_pipe\Render\BigPipe::sendContent()\Drupal\Core\Asset\AttachedAssetsInterface::setAlreadyLoadedLibraries()\Drupal\Core\Asset\AssetResolver::getLibrariesToLoad()getCssAssets()andgetJsAssets()to the decorated service.\system_js_settings_alter()ajaxPageStatelibraries are built.\Drupal\Core\StackMiddleware\AjaxPageState::parseAjaxPageState()Comment #17
ambient.impactAlright, 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:
preprocess: falsefor 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 therefreshless_turbo/refreshlessandrefreshless_turbo/turbolibraries so that we could take advantage of core's minification and gzipping.refreshless_turbo/refreshlessandrefreshless_turbo/turbolibraries 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.HtmxResponseAttachmentsProcessor) to hopefully remove the duplication.gin.cssthat gets attached to every page and uses a fixed weight value on itstabs.csslibrary 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 thegetCssAssets()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.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.
Comment #18
ambient.impactUpdating title as we now know what needed to be done and why.
Comment #19
ambient.impactHeavy rewrite of issue summary reflecting current understanding of the problem and solution.
Comment #20
ambient.impactAdded remaining tasks and follow up tasks.
Comment #21
ambient.impactMinor grammar fix.
Comment #22
ambient.impactComment #23
ambient.impactUpdated status of a few items.
Comment #24
ambient.impactReordered "Remaining tasks" and "Follow up tasks", and moved some stuff between them where they make more sense.
Comment #25
ambient.impactLink to #3401146: Turbo: Automated tests for test details.
Comment #26
ambient.impactAdded a few remaining tasks from my comments.
Comment #27
ambient.impactAdded References section with links to the HTMX module.
Comment #28
ambient.impactRemoved session item (not actually true) and added page cache request policy item.
Comment #50
ambient.impactComment #51
ambient.impactI'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
ajaxPageStatebecause outputting it on every page results in certain generated URLs that have a query string (e.g. pager,admin/configcompact 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.