Allow loading CSS and JavaScript directly from templates

Our current aggregation strategy is based on libraries, which was a major improvement from the Drupal 7 method and has unblocked a lot of front end performance issues like #956186: Allow AJAX to use GET requests and #1014086: Stampedes and cold cache performance issues with css/js aggregation. Moving back to inclusion of individual files would complicate this a lot.

#4++

It's not clear to me what problem is solved by not using asset libraries. Can you give a concrete example?

Thanks, that helps a lot! I now see a concrete use case, and see the ways it could break.

But as far as I can tell, there's no requirement to have a theme override only a CSS file; we could just as well demand that the theme overrides the entire asset library associated with that template. That way, we can continue to rely just on asset libraries, and not go back to the world of both asset libraries and individual assets.

I suspect this is less appealing because this further increases the amount of work for a theme developer?

Looks like we're having convo in two places.

we could just as well demand that the theme overrides the entire asset library associated with that template.

If we can do that, like have the library only callable by the theme that defines it, that would serve the need; however, I worry that would get confusing. The nice part of attaching an asset directly, like css/my-style.css is it would be obvious to anyone, even the inexperienced, what they'd have to do.

I suspect this is less appealing because this further increases the amount of work for a theme developer?

It's not just work. Creating a library isn't a lot of work, and I'd challenge anyone saying so, but it does create two issue. One, it requires a fair amount of knowledge of how theming works in Drupal to get used to. Two, it requires foreknowledge. The libraries have to be setup before anything can work. Working with assets directly can make setting up things like a component system simpler, because you can have templates and assets bundled without dealing with the libraries. (which are independent from the component) The effect is to simplify the mental model.

Moving back to inclusion of individual files would complicate this a lot.

Duly noted. I think it would be acceptable to put some restrictions on it. We've often gone out of our way to add more flexibility than needed in some places. Less flexibility, but consistent and reliable is a good tradeoff. I'm not sure though what that would be.

BTW, this is actually the spiritual successor to #2398331: Add the ability to attach asset libraries directly from a template file :)

Discussed this with @lauriii at Drupal Dev Days.
Seeing this from an end-user perspective: the only actual difference would be that next to attach_library, we would now have a more granular approach by saying attach_css and attach_js.

This more granular approach sounds like a really valuable thing, as that implies that
1) We have better naming of what we are actually doing as a themer not sure what a library contains.
2) We move closer in our naming to the fundaments of the web (HTML/CSS/JS). When writing HTML (in twig) we attach CSS and JS. (makes it super readable)
3) This opens up the possibilities of progressive rendering: Drupal Core in the future could decide to allow progressive rendering by inlining JS and CSS directly as little files in the HTML output. Which would be a massive push forward in way how we think about theming.
4) With compony.io I am making a push towards seeing the theming-layer as a mere collection of components that are self-sufficient. Having a clear separation between library / css / js in terms of assets would make everything a whole less complicated, as that would mean lower the amount of work that needs to happen to attach an asset to a component.
5) Not a lot of themers dares to unset libraries, because it's hard to be sure what exactly you are unsetting. Having this more fine grained control would mean more flexibility and better readability.

This came up in #3064854: Allow Twig templates to use front matter for metadata support, and actually an alternative implementation proposal was made there. See the issue summary and #3064854-35: Allow Twig templates to use front matter for metadata support.

I kind of have to agree with #4. This kind of defeats the whole point of moving everything over to libraries.

That being said, this issue (at its core) is basically discussing the concept of components; which more or less duplicates the related issues I'm attaching now.

I'm not entirely sure this is something that can or should be done at this time (in 8.x).

At the very least, I think any "component" based issue really needs to be postponed on something like #2869859: [PP-1] Refactor theme hooks/registry into plugin managers.

We really need to focus on modernizing the core APIs of the theme system first before we attempt to shift theming paradigms as a whole.

I agree with #4, but #9 addressed those concerns.

Not really, it merely skirts around the issue by essentially hacking in dynamic libraries; something templates were never really designed to do.

Yes, we have the attach_library() Twig function, but that only attaches existing libraries that are already defined elsewhere.

You asked me to create a POC in #3064854-43: Allow Twig templates to use front matter for metadata support.

This issue would require, at a minimum, the inclusion of #3075427: Create TemplateDiscovery for plugin managers to use if we were to go this route.

It would then need to actually create some sort of plugin manager to actually collect discovered templates.

All this to mean that this issue is essentially akin to introducing the concept of components in core (which we already have issues for).

I'm not saying that this isn't needed or desired, but the theme system will need some serious overhaul for this to all work properly.

Regardless, this issue doesn't entirely seem like the biggest priority right now.

The metadata definition may also help to #3063009: Add the ability to attach drupalSettings directly from a template file

There's working base class to use in #3075427: Create TemplateDiscovery for plugin managers to use

I want to give a huge +1 to this from a core developer perspective

If themers had the ability to attach CSS and JS without creating a library, core developers would have much more flexability to modify markup and styling, which would enable more rapid innovation within the front-end of Drupal. This is extremely important now that Drupal 10 will not be supporting legacy browsers.

Forcing core themes to use the library system sets an expectation that the library will be forever available and not change. This negatively affects our ability to improve the markup, styling, and resilience of the styles.

An example of this is Drupal’s tabs. Themes should not have to create their own styles for this (unless they want to simply updating the colors and spacing), however the tabs’ default markup is inadequate (which includes the non-BEM CSS classes).

I want to give a huge +1 to this from an general development perspective

As a front-end developer who works with clients on a daily basis, having to create a single one-use library for each and every component I develop, creates a very negative development experience. I should not have to jump around to multiple places within my codebase just to get a single component working (example: template directory, the CSS directory, and to the libraries file). While this doesn’t 100% solve this issue, the ability to not create a library takes a huge step in fixing this.

Implementation Details

• Does the `style.css` in the example reference the same directory that the template exists in? Should we do something like`{{ attach_css('@olivero/css/styles.css') }}`? Ideally we could do both.
• I think the ability to declare dependencies within these functions would be useful
• I’d personally prefer `attach_js` over `attach_javascript`. Less typing!

As I understand it, this is a required step of #2659890: [Policy] [Plan] Drupal 9 and 10 markup and CSS backwards compatibility (and is listed in that summary as such), so setting this issue's parent to that one, and promoting this to Major accordingly.

The main concern is that allowing attaching individual assets rather than libraries makes calculating required assets for a page more expensive.

Yes, I agree with this concern.

To avoid this problem, Wim proposed to redesign the Twig functions (attach_css() + attach_js()) from the issue summary so that all assets would be loaded with a single function call (e.g. attach_assets()).

+1. Not sure if there's agreement here on that or not, but if/when there is, can we update this issue's summary accordingly, especially the example code in the API changes section?

During Twig compilation, we generate one (immutable) asset library per [attach_assets()] call...They would get auto-generated names such as THEMENAME/component--COMPONENTNAME--index.

Does the generated library name need to be named with the theme and component names? If Twig extension functions have ready access to that info, then great, but if not, then would it be easier to instead name the library as a hash of the asset names?

This issue would require, at a minimum, the inclusion of #3075427: Create TemplateDiscovery for plugin managers to use if we were to go this route. It would then need to actually create some sort of plugin manager to actually collect discovered templates.

It looks like #3075427: Create TemplateDiscovery for plugin managers to use has some great use cases in that issue, but I don't understand why it's a requirement for this one. Can you elaborate on that?

I am not sure if this the direction we want to go, but one of the goals of CL Components is precisely to simplify maintenance of CSS, JS, and assets.

As @lauriii points out we would benefit from communicating that when you override X you are taking over of X CSS, JS, and assets as well. It does not make sense to have that burden perched on the framework.

The solution we arrived at in CL Components is not to allow inclusion of CSS and JS assets directly in the templates, but quite the opposite. Developers associate CSS and JS to Twig templates by adding them to the component folder. The documentation shows this (abbreviated for more focus on the current issue):

[PROJECT_ROOT]/...
    |- ... organize your components as you see fit
        |- my-component-machine-name
            |- my-component-machine-name.twig (required)
            |- assets
                |- css
                    |- some-styles.scss
                    |- some-more-styles.css
                |- js
                    |- some-javascript.js
                    |- other-javascript.js
                |- img
                    |- img1.png

Then the module creates a library dynamically cl_components/a324dlkjaef30 with the JS and CSS. The library gets attached automatically whenever anyone includes my-component-machine-name.twig (using a custom Twig loader). This way, the front-end developer never has to worry about aggregation, backwards compatibility, ...

Now if someone wants to take over my-component-machine-name the implementation forces them to copy the whole folder, which implies ownership of JS and CSS.

I am not sure weather or not this helps in this issue, but I thought to share an alternative point of view on the matter.

Additionally (and a bit off topic, hence the separate comment), all components need to have a metadata.json file. Which includes (among other more relevant bits) the status of the component. This would also help with version upgrades and BC, because the site would know if you are rendering a component that has been marked as "status": "DEPRECATED" by upstream.

#26 sounds VERY convincing to me.

This sounds REALLY interesting, and is how we organise assets in our custom themes at Annertech (either via using Storybook or our own base theme). Here's an example of our 'accordion' component directory:

What I love about this is that I don't need to go searching for CSS in one directory, JS in another, etc.

However ... sorry about this, but I'm not convinced yet that for _Drupal Core_ we should take the same approach. If we have patterns in Drupal core for a modal for example, and then we decide there is something we'd like to add for accessibility (a newly release aria attribute for example), anyone who simply wanted to extend the twig template will not get this new update since they will also have taken over the CSS and JS and HTML for that component.

An approach like this for Drupal Core will - I think (happy to be corrected here) - mean a lot more maintenance for developers if they have to take control of a lot more than they intended.

---

Edit: I don't have a problem with all the parts of a component (HTML, CSS, JS, etc) being in the same directory for Drupal Core (I love that idea), my only concern is with the idea that "you take ownership of the full component if you take ownership of any part of it".

I don't have a problem with all the parts of a component (HTML, CSS, JS, etc) being in the same directory for Drupal Core (I love that idea), my only concern is with the idea that "you take ownership of the full component if you take ownership of any part of it".

If we have the metadata.json file as described in #27, how about supporting a "parent" or "extends" key to extend another component? Like, whenever the child component gets used, it automatically includes the assets from the "parent" component.

@markconroy

my only concern is with the idea that "you take ownership of the full component if you take ownership of any part of it".

The arguments for doing this are documented in #6. How do you respond to them?

This patch adds a working add_css() that adds the asset via the library system. It requires the full asset path at the moment, but that (and an add_js) can be added if this seems to resemble the direction to go in.

The asset path is currently snuck into the library name. We'd need something a little slicker if we wanted to add options like weight, and that approach may weird some folks out in general.

Either way, here's 3k that demonstrates this can happen without moving the earth.

I am working on this now in the scope of CL Components.

#3293455: Allow altering components created elsewhere

Pretty neat.

Few comments,

1. Passing the path in the library name might get troublesome with regard to asset URL length if there are a lot of templates that uses that, even with #3303067: Compress aggregate URL query strings. So we'll need the slicker solution right away unfortunately.
2. I'd use the names from the IS for the functions (attach_js/attach_css) so that it's not confused with the old/removed drupal_add_js/drupal_add_css methods.

cross post, used an old tab to write the message :o

relevant change notices for context on #38:
https://www.drupal.org/node/3301716
https://www.drupal.org/node/2888767

This is the way I am proposing to do it in CL Components. See attached video (3min 38sec).

• This adds (and actually requires) prefixing the asset path with @module, @theme or @core.
• the attach_js and attach_css accept a second argument of a config object that works the same as the config object for an asset path in a .libraries.yml file
• The library name length is no longer a concern as there isn't a bunch of extra data attached to it. During CSS/JS attach, we get all the necessary info by parsing the twig file directly.

Definitely needs tests. Hope to work on those next 😎

• Added basic tests
• Changed service requests to dependency injection
• Added docblocks
• Other mix fixes

Testing needs to go further than the basic checks added here, but it's a good foundation for how to write them.

Fixed failed commands on #43
Attached interdiff patch

This adds JS to testAttachToPageTemplate and adds the new dependencies for AssetResolver and TwigExtensions to their unit tests, which was the cause of the Commands Failed in #43 (which at least for me didn't happen when I ran the commit script locally).

This also fixes a JS attach issue that was exposed by adding this test.

This does not take #44 into account as that attempted to fix the check in #43 by adding arbitrary commands to a constructor vs the actual classes it required.

More tests are still needed including: subthemes, use on non-base templates, passing in options, and likely even more than that. Work will continue.

Test coverage expanded, including confirmation that the assets aggregate as if they were added as part of a traditional library.

1. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
   @@ -2,6 +2,9 @@
   +use Symfony\Component\Process\Exception\LogicException;
   

   Surely this is not the LogicException you're looking for. 😎

2. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
   @@ -72,14 +94,30 @@ class AssetResolver implements AssetResolverInterface {
   +   * @param \Drupal\Core\Template\TwigEnvironment $twig
   +   *   The Twig environment.
   +   * @param \Drupal\Core\Theme\Registry $theme_registry
   +   *   The theme registry.
       */
   -  public function __construct(LibraryDiscoveryInterface $library_discovery, LibraryDependencyResolverInterface $library_dependency_resolver, ModuleHandlerInterface $module_handler, ThemeManagerInterface $theme_manager, LanguageManagerInterface $language_manager, CacheBackendInterface $cache) {
   +  public function __construct(LibraryDiscoveryInterface $library_discovery, LibraryDependencyResolverInterface $library_dependency_resolver, ModuleHandlerInterface $module_handler, ThemeManagerInterface $theme_manager, LanguageManagerInterface $language_manager, CacheBackendInterface $cache, TwigEnvironment $twig, Registry $theme_registry) {
   

   Shouldn't $twig and $theme_registry default to NULL? And do we have test coverage for the deprecation notices that will be raised if they're not passed?

3. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
   @@ -72,14 +94,30 @@ class AssetResolver implements AssetResolverInterface {
   +      @trigger_error('Calling AssetResolver::__construct() without the $twig argument is deprecated in drupal:10.1.0 and will be required in drupal:11.0.0.', E_USER_DEPRECATED);
   

   Pro-tip: I usually use __METHOD__ . '() in deprecation notices like this, so that the fully qualified class name is displayed without needing to type it all in.

4. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
   @@ -107,6 +145,106 @@ protected function getLibrariesToLoad(AttachedAssetsInterface $assets) {
   +  /**
   +   * Creates a definition for dynamically generated Twig libraries.
   +   * @param $name
   +   *   The library name.
   +   * @param $definition
   +   *   The library definition
   +   */
   +  protected function componentDefinition($name, &$definition) {
   

   Few minor things:

   • There needs to be an empty line between the one-line description and the param list.
   • Should $name and $definition be type hinted?
   • Should this method have a return type hint?
   • The name componentDefinition() seems a little value. Maybe createComponentDefinition() would be clearer.
   • Why does $definition need to be passed by reference? Is there any way to avoid that?

5. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
   @@ -107,6 +145,106 @@ protected function getLibrariesToLoad(AttachedAssetsInterface $assets) {
   +    if (str_starts_with($name, 'TWIG_ATTACH---')) {
   

   Depending on how often it's reused, it might make sense for TWIG_ATTACH--- to be a class constant.

6. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
   @@ -107,6 +145,106 @@ protected function getLibrariesToLoad(AttachedAssetsInterface $assets) {
   +      [$theme_hook, $file_type] = explode('>>', $name);
   

   This looks like $name is expected to be in a very specific format. Are we validating that in some way?

7. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
   @@ -107,6 +145,106 @@ protected function getLibrariesToLoad(AttachedAssetsInterface $assets) {
   +      assert(in_array($file_type, ['css', 'js']), 'Only JS and CSS files should be attached to a template');
   

   We should always use strict comparison with in_array(). Also, do we want to normalize $file_type here by calling strtolower()?

8. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
   @@ -107,6 +145,106 @@ protected function getLibrariesToLoad(AttachedAssetsInterface $assets) {
   +      $info = $this->themeRegistry->get()[$theme_hook];
   

   🤔 Looking at ThemeRegistry::get(), it seems that it expects a string to be passed, rather than returning an array like this. Should this be $this->themeRegistry->get($theme_hook)? If so, then the fact that this didn't fail tests points to some missing coverage.

9. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
   @@ -107,6 +145,106 @@ protected function getLibrariesToLoad(AttachedAssetsInterface $assets) {
   +      $template_file = $info['path'] . '/' . $info['template'] . '.html.twig';
   

   Are we sure the path and template keys will be set? What happens if they're not?

10. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
    @@ -107,6 +145,106 @@ protected function getLibrariesToLoad(AttachedAssetsInterface $assets) {
    +        if (str_contains($node_string, 'attach_' . $file_type)) {
    

    Would str_starts_with() make more sense here?

11. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
    @@ -107,6 +145,106 @@ protected function getLibrariesToLoad(AttachedAssetsInterface $assets) {
    +            if ($node instanceof PrintNode && $node->getNode('expr') instanceof FilterExpression) {
    +              $expr = $node->getNode('expr');
    +              $expr_node = $expr->getNode('node');
    +              if ($expr_node instanceof FunctionExpression && $expr_node->getAttribute('name') === 'attach_' . $file_type) {
    +                foreach ($expr_node->getNode('arguments')->getIterator() as $index => $arg) {
    +                  if ($arg instanceof ArrayExpression) {
    +                    $pairs = $arg->getKeyValuePairs();
    +                    foreach ($pairs as $pair) {
    +                      $value = $pair['value']->getAttribute('value');
    +                      if ($index === 0) {
    +                        $files_to_add[] = $value;
    +                        $options[$value] = [];
    +                      }
    +                      else {
    +                        $key = $pair['key']->getAttribute('value');
    +                        $options[end($files_to_add)][$key] = $value;
    +                      }
    +                    }
    +                  }
    +                }
    +              }
    

    OK, not gonna lie: this code is terrifying and I don't understand what it's doing, so I'm basically just taking it on faith that it works. I feel like abstracting this into its own method and dousing it generously with comments would be helpful for future maintenance.

    Also, this seems like quite a heavy operation. Are we caching this in any way?

12. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
    @@ -107,6 +145,106 @@ protected function getLibrariesToLoad(AttachedAssetsInterface $assets) {
    +  public function assetPath($path) {
    

    Why no type hints?

    Also, the name assetPath() seems a little vague in relation to what the doc comment says this method does. How about getAssetPath() or computeAssetPath()?

13. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
    @@ -107,6 +145,106 @@ protected function getLibrariesToLoad(AttachedAssetsInterface $assets) {
    +    if ($this->moduleHandler->moduleExists(substr($path_prefix, 1))) {
    

    Maybe we should assert that $path starts with @, since I could easily see that little quirk being missed.

14. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
    @@ -107,6 +145,106 @@ protected function getLibrariesToLoad(AttachedAssetsInterface $assets) {
    +      return str_replace($path_prefix, $base_themes[substr($path_prefix, 1)]->getPath(), $path);
    

    We're calling substr($path_prefix, 1) several times. It should probably be its own variable, which would also improve readability.

15. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
    @@ -107,6 +145,106 @@ protected function getLibrariesToLoad(AttachedAssetsInterface $assets) {
    +      throw new LogicException("Asset prefix $path_prefix does not correspond to an installed module, theme or subtheme.");
    

    Shouldn't "subtheme" be "base theme" in this context?

16. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -82,13 +90,21 @@ class TwigExtension extends AbstractExtension {
    +  public function __construct(RendererInterface $renderer, UrlGeneratorInterface $url_generator, ThemeManagerInterface $theme_manager, DateFormatterInterface $date_formatter, FileUrlGeneratorInterface $file_url_generator, Registry $theme_registry) {
    

    Same question here about whether $theme_registry should default to NULL (and related test coverage).

17. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -365,6 +383,80 @@ public function attachLibrary($library) {
    +  private function attachAsset(array $assets, string $type, $context) {
    

    Missing return type hint?

18. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -365,6 +383,80 @@ public function attachLibrary($library) {
    +      $registered_hooks = $this->themeRegistry->get();
    

    As before, I'm not sure ThemeRegistry::get() is meant to be called without any arguments.

19. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -365,6 +383,80 @@ public function attachLibrary($library) {
    +      // This is a temporary library with a crazy name that is later parsed to
    +      // add the asset.
    +      $theme_library_name = "$active_theme/TWIG_ATTACH---component--$theme_hook>>$type>>$index";
    

    This could use an @see pointing to the place where the library name is parsed.

20. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -365,6 +383,80 @@ public function attachLibrary($library) {
    +   *   Each CSS file path should be prefixed with one of the following
    +   *    - @THE-THEME-MACHINE-NAME if the file is in a theme
    +   *    - @THE-MODULE-MACHINE-NAME if the file is in a module
    +   *    - @core if the file is not in a theme or module.
    +   *    These prefixes will resolve to the full theme/module/core paths.
    

    I think this could use some examples.

    Also, we should be clear about whether the module or theme you reference in the asset name needs to be installed or not.

21. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -365,6 +383,80 @@ public function attachLibrary($library) {
    +   *   Each JavaScript file path should be prefixed with one of the following
    +   *    - @THE-THEME-MACHINE-NAME if the file is in a theme
    +   *    - @THE-MODULE-MACHINE-NAME if the file is in a module
    +   *    - @core if the file is not in a theme or module.
    +   *    These prefixes will resolve to the full theme/module/core paths.
    

    Same comments as for attachCss().