Toronto
Account created on 24 January 2011, almost 14 years ago
#

Merge Requests

More

Recent comments

🇨🇦Canada ambient.impact Toronto

I recreated the change on 11.x, but since I can't edit MR !10210 (and I'm tired), I created a new one. Can someone close MR !10210 for me?

🇨🇦Canada ambient.impact Toronto

My original implementation of dark mode toggling was primarily CSS using @media (prefers-color-scheme: dark) {}, which not only would have avoided having to store anything by default, but also removed the additional problems with having to check storage with JavaScript, i.e. no chance of brief flashes of light mode unless you used an inline script which is a problem with CSP, etc. See #3161904-27: Auto Darkmode option for my concerns at the time.

🇨🇦Canada ambient.impact Toronto

That's what I get for having the tab open with the form state saved. 🤦

🇨🇦Canada ambient.impact Toronto

An alternative if you don't want to install Ultimate Cron just to disable Warmer cron is the Hux module 's ability to replace a hook implementation like we did on Omnipedia.

🇨🇦Canada ambient.impact Toronto

I can confirm that I'm also seeing this problem where a class that only uses ReplaceOriginalHook didn't get discovered no matter how many container rebuilds or full cache clears I tried, and I thought I was losing my mind until it dawned on me to search the issue queue. I can also confirm that adding a #[Hook('...')] to the class does seem to cause it to be discovered.

🇨🇦Canada ambient.impact Toronto

Ah, I see I'm not the only one that was utterly confused by the cryptic "LoadJS" error. It turns out the library did this to keep the size down, which I personally feel was a bad move because it's so confusing and non-helpful. I opened a pull request to change it, but since they still want to keep the size down, they're only willing to throw the bundle ID as the error message rather than a fully descriptive thing that says why it was thrown. Glad to see this is being handled on the Drupal side.

🇨🇦Canada ambient.impact Toronto

Not to dunk on the author of that even more, but I'm in agreement with @fathershawn, and would also highlight:

An attractive future direction might be to re-implement Htmx in React:

  • The server sends a JSON blob that React converts into virtual DOM
    components.

...which baffles me because it sounds like this person doesn't understand the core philosophy behind htmx and similar approaches. Sending a JSON blob for React to convert into a virtual DOM has nothing to do with htmx, and it's in fact entirely antithetical to htmx as I understand it.

🇨🇦Canada ambient.impact Toronto

Even after applying the patch from the merge request and clearing caches, I'm still getting the fatal error. Is there a step I'm forgetting to get this to work?

🇨🇦Canada ambient.impact Toronto

It's worth taking a quick look at Turbo, if only to get a sense of what we would have to recreate with HTMX. I did a bit of poking around the HTMX docs just now and realized they have a guide for migrating from Hotwire Turbo, so that'll be handy.

🇨🇦Canada ambient.impact Toronto

@fathershawn Most of that I'm aware of. I think the biggest challenge in implementing the current functionality in HTMX is that we'll have to re-implement a lot of stuff that Hotwire Turbo does out of the box, but building on your module will help get there a bit faster when I (or someone reading this?) have the time and energy for it.

My impression is that HTMX seems to more of a low level toolkit that you build with - would you say that's accurate?

🇨🇦Canada ambient.impact Toronto

If someone is willing to implement it in a robust and reliable way, I'd be happy to review and merge it. I can see the value of it, but my time and energy are limited right now so it's not a priority for me.

🇨🇦Canada ambient.impact Toronto

I took a quick look but Pinto seems to require more set up than I would like. I feel that it's missing what I love about Hux: I can just install Hux, drop one or more classes into src/Hooks with concise PHP attributes, and it automagically works. I don't know enough about the inner workings of Drupal's theme system, so maybe that's just not currently possible, but I feel like Hux has the winning formula from a DX perspective.

🇨🇦Canada ambient.impact Toronto

Doing a bit of research on this and came across the Preprocess module , which seems pretty straightforward to use until core or someone else comes up with a better solution. It's not quite as elegant as Hux, but it's not terrible either.

🇨🇦Canada ambient.impact Toronto

We still need a proper build process as detailed in the issue summary but the commits above are good enough for now and accomplish the immediate goal.

🇨🇦Canada ambient.impact Toronto

Removed 📌 Turbo: Configuration and form for toggling Turbo features Active as it's not really relevant to a minimum viable implementation right now, though might be done down the road.

🇨🇦Canada ambient.impact Toronto

Added postponed issues and ongoing issues lists, and moved linked issues where appropriate.

🇨🇦Canada ambient.impact Toronto

Removed some lingering old stuff that's no longer relevant.

🇨🇦Canada ambient.impact Toronto

Significant restructure of issue summary; moved the old stuff to a section at the end as archived notes.

🇨🇦Canada ambient.impact Toronto

Going to close this as won't fix because there's no realistic way we could do this without breaking various things.

🇨🇦Canada ambient.impact Toronto

Added item for Drupal\refreshless_turbo\Value\RefreshlessRequest.

🇨🇦Canada ambient.impact Toronto

Added list item for page cache request policy.

🇨🇦Canada ambient.impact Toronto

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

🇨🇦Canada ambient.impact Toronto

Added CSS and JS aggregation list items.

🇨🇦Canada ambient.impact Toronto

Changed ⌛️ emoji to ⏳ as the former is a finished hourglass while the latter is what we want: an in-progress hourglass.

🇨🇦Canada ambient.impact Toronto

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

🇨🇦Canada ambient.impact Toronto

Whoops, it looks like I accidentally replaced the issue summary with my comment. One of the maintainers should revert that if possible. 😬

🇨🇦Canada ambient.impact Toronto

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

🇨🇦Canada ambient.impact Toronto

In my case, I have a couple of sites with complex content that makes heavy use of the caching API (cache tags and cache contexts) and we try to avoid clearing the cache unless absolutely necessary. We even have a custom drush.yml with this:

command:

  updatedb:
    options:
      # Don't clear the cache by default to avoid unnecessary work.
      #
      # @see https://www.bounteous.com/insights/2020/03/12/automated-drupal-deployment-and-rollback-recipes/#don%E2%80%99t-clear-all-caches-instead-selectively-clear-what-you-need
      cache-clear: 0

Which I suspect may be part of the reason we also got hit with this Drupal\config_split\Plugin\ConfigFilter\SplitFilter error, despite following the 2.0.0 instructions and had drupal/config_filter required explicitly. Thankfully, this happened on a staging site so I could just restore a database back up and try a few things. What ended up working in our case to avoid a full cache clear was:

  1. Upgrade Config Split in a dev environment; update database; export config; commit/push.
  2. On the target site (staging/production) do drush pm:uninstall config_split
  3. Deploy the updated codebase and config files.
  4. drush -y updb and then drush -y config:import (our deployment process does this automatically)
  5. Config Split 2.x is now installed and using the 2.x config without a problem.
🇨🇦Canada ambient.impact Toronto

The workaround I've committed is okay for now, but still results in brief flashes of incorrect specificity, so I'm going to set this as postponed for the time being.

🇨🇦Canada ambient.impact Toronto

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 [PP-1] Allow setting aggregation groups for js files in library definitions Postponed . 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 work 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 🐛 Turbo: CSS files merged into the on Turbo navigation always appended instead of preserving full page load order, causing CSS specificity issues Active 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.

🇨🇦Canada ambient.impact Toronto

Merged and setting back to active. Thank you, bot.

🇨🇦Canada ambient.impact Toronto

Right you are. The param hints have been reverted.

Also ignore the 1.0.x branch in the fork. Accidentally pushed to that because I somehow had that set as the upstream instead of my own. Had a senior moment.

🇨🇦Canada ambient.impact Toronto

ambient.impact changed the visibility of the branch 1.0.x to hidden.

🇨🇦Canada ambient.impact Toronto

I merged in the latest 8.x-1.x changes, i.e. up to 8.x-1.4-rc1 ;)

🇨🇦Canada ambient.impact Toronto

ambient.impact made their first commit to this issue’s fork.

🇨🇦Canada ambient.impact Toronto

Rerolled the patch because the previous couple of patches broke Drupal\Tests\media\Functional\testResourceDataAlter(); notable changes:

  1. Fixed this syntax error that snuck in; see if you can spot it ;) : $resource_url = $this->container->get('media.oembed.resource_fetcher')+ ->fetchResource('https://publish.twitter.com/oembed?url=https://twitter.com/Dries/status/999985431595880448');
  2. Added void return type to testResourceDataAlter() for consistency with other core changes to tests.
🇨🇦Canada ambient.impact Toronto

So I tried to implement a solution based on @catch's idea 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.

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

Production build 0.71.5 2024