Add libraries-extend to themes' *.info.yml

Your initial proposed syntax should probably be an array format (or allow for an array format)

If you wanted to extend a library by adding more than one library, you'd need something like:

# Extend drupal.user: add assets from classy's user libraries.
libraries-extend:
  core/drupal.user: 
    - classy/user1
    - classy/user2

I am not sure what the Drupal YAML standards are for variable elements (can a node be a string OR an array of strings.)

How about if we wanted to extend a library with just a component from a theme or elsewhere. We could do something like:

# Extend drupal.user: add assets from my theme's custom css
libraries-extend:
  core/drupal.user: 
    css:
      theme:
        my/custom/css/button.css: { }

This allows to add some new css or js to the existing library, without having to necessarily define a new library.

@almaudoh

I still think that it needs to be an array per library:

# Extend drupal.user: add assets from my theme's custom css
libraries-extend:
  core/drupal.user: 
    - css:
        theme:
          my/custom/css/button.css: { }

I started by following the overrides code, but it diverges from our needs pretty quickly.

I'm not ready to patch for this deeper extension syntax, but right now I am playing with injecting library dependencies for an library-extensions. There would be some notable behaviour that would come with such an approach, the most significant being that dependencies are loaded before the library that they're attached to.

Is this something that we're thinking about just for one of the subthemes, or are we going right for the core? If it's the core then I can look at altering the library discoverer right away, but if it's not in the raw core, then it will require overloading or adding more code.

So where do we stand on the syntax here. Considering that there is mention that this particular capability may be needed sooner rather than later, we could start work on a patch here.

So why did we split this off into its own thing instead of making it part of libraries-override? It seems like override just needs an add asset syntax.

So combining #1 and #5, we could have something like:

libraries-extend:
  core/drupal.user:    # The name of the library being extended
    - classy/user1      # Add all the assets defined by 'classy/user1'
    - classy/user2      # and also all those defined by 'classy/user2'
    - css:          # Add a specific asset available in this theme
        theme:
          my/custom/css/button.css: { }

This looks sensible to me and easy for everyone to understand.

@davidhernandez: There is a slight difference to the libraries-override syntax which doesn't specify attributes for the assets. But also it was to make the patches easier to review.

So why did we split this off into its own thing instead of making it part of libraries-override? It seems like override just needs an add asset syntax.

Because we wanted to keep the scope as small as possible over there, I think that was mostly discussed/agreed upon in IRC, but #2451411-37: Add libraries-override to themes' *.info.yml and #2451411-63: Add libraries-override to themes' *.info.yml suggest this at least.

If it were me I would keep it stupidly simple and just allow extending a library with another library for now.

The use case of directly adding CSS is very nice, but could internally be still be implemented as a hash over the contents of the definition.

core/autogenerated_[HASH]

and create a library on the fly.

Overall however I don't see much difference in:

libraries-extend:
  core/drupal.user:    # The name of the library being extended
    - classy/user1      # Add all the assets defined by 'classy/user1'
    - classy/user2      # and also all those defined by 'classy/user2'
    - css:          # Add a specific asset available in this theme
        theme:
          my/custom/css/button.css: { }

vs.

libraries:
  - foo:
    - css:          
        theme:
          my/custom/css/button.css: { }

libraries-extend:
  core/drupal.user:    # The name of the library being extended
    - classy/user1      # Add all the assets defined by 'classy/user1'
    - classy/user2      # and also all those defined by 'classy/user2'
    - mytheme/foo    # Just add the library from above

While being way more flexible to work with.

e.g. another module or subtheme could decide to just add something to mytheme/foo - regardless if that is used in one or in several libraries as extension.

override is a different "beast" in that that should allow to override / remove or change specific CSS files.

This issue should IMHO be purely about libraries extending other libraries and nothing more - and hence keep things really simple.

+1 to #11. So basically we're looking at libraries-extend: in foo.libraries.yml instead of foo.info.yml. But how do we explain to someone new to Drupal 8 why libraries-extend is in foo.libraries.yml while libraries-override are in foo.info.yml.

#13, #14: Yes, I was unclear there.

I meant:

- libraries to *.libraries.yml
- libraries-extend to *.info.yml

Modules can use the good ol' hook_library_info_alter() to do the same.

Ok. So #1, #11 and #15 makes the implementation relatively simple.
Here's a patch rolled on top of the latest patch at #2451411: Add libraries-override to themes' *.info.yml. The do-not-test.patch is the patch for this issue with one test. More tests still needed.

Some key points:

1. I guess libraries-override takes precedence over libraries-extend. But the implementation libraries-extend will happen after override, so overrides must always take place on the library where it is defined, not where it is extended.
2. Does libraries-extend need to check if there is already existing assets as it is trying to extend and throw an exception? That might be good for TX, but I don't know
3. Do we need to do a recursion check? For example if a library tries to extend another library that is extending the former library. That results in an infinite recursion. May be far fetched... :)

Fixed the test fails and added some docs. Still needs more tests.

Overall looks great, one detail:

+++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
@@ -185,6 +185,21 @@ public function getActiveTheme(Extension $theme, array $base_themes = []) {
+    foreach ($base_themes as $base) {
+      if (!empty($base->info['libraries-extend'])) {
+        foreach ($base->info['libraries-extend'] as $library => $extend) {
+          $values['libraries_extend'][$library] = $extend;
+        }
+      }
+    }
+    // Add libraries extend declared by this theme.
+    if (!empty($theme->info['libraries-extend'])) {
+      foreach ($theme->info['libraries-extend'] as $library => $extend) {
+        $values['libraries_extend'][$library] = $extend;
+      }
+    }

I thibk this should extend the libraries to extend and not override.

Or merge in other words.

I.e. if a base theme extends a component, why should the theme have to do the same and not just extend more?

#22.1 Great point! Fixed. Also added some more tests. This should be ready now.

Wow, great work here! :)

#2451411: Add libraries-override to themes' *.info.yml landed. Reuploading without that part, i.e. reuploading #23's "do-not-test" patch. Zero changes made.

This looks excellent. I was only able to find nitpicks, fixed those myself.

1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryCollector.php
   @@ -124,8 +126,50 @@ protected function getLibraryDefinitions($extension) {
   +        // If libraries are not overridden, then apply libraries extend.
   

   s/libraries extend/libraries-extend/

2. +++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
   @@ -219,4 +221,14 @@ public function getLibrariesOverride() {
   +   *   The list of libraries extend.
   

   s/libraries extend/libraries-extend definitions/

3. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
   @@ -185,6 +185,35 @@ public function getActiveTheme(Extension $theme, array $base_themes = []) {
   +    // Get libraries extend declared by base themes.
   ...
   +    // Add libraries extend declared by this theme.
   

   Libraries extensions (so it is consistent with the wording in the preceding code which says "libraries overrides").

Committer: no need to credit me for these language nitpicks.

From the beta evaluation:

1. This would be a huge themer experience win.
2. This would be a big front-end performance win: rather than loading all the CSS extending existing CSS all the time, this ensures it's only loaded when necessary.
3. Last but not least: this is crucial BigPipe (in core or contrib), because it means a theme's CSS extending other CSS is loaded not as part of the initial theme CSS, but as part of the CSS for the actual component. Remember that the component may be rendered *after* the initial page! So this ensures that we don't end up with 1. theme CSS, 2. component CSS, with 2 overriding 1, which is a problem in HEAD. With support for this, we completely avoid that problem.
4. The last point also quite clearly shows how this makes Drupal 8 ready for a Web Component world.

We have strong +1 from Cottser.

Strong RTBC + 1

Yes, this usage pattern is crucial for BigPipe, AJAX, ESI + Co and one of the big enablers.

No more !important in the theme for AJAX CSS loaded afterwards ...

Moved Wim's strong arguments in #26 up higher in the IS.

Can we get a quick followup to actually use this in Classy... the theme has an @todo on this issue.

We've got test coverage and this is an addition so less risky than a change. Committed b95d2c8 and pushed to 8.0.x. Thanks!

WOOOHOOO! Thank you!

Can we get a quick followup to actually use this in Classy... the theme has an @todo on this issue.

Done: #2580255: Remove (classy|seven)_library_info_alter() in favor of libraries-extend.

This should have a change record.

I'll see if I can work on one later, but anyone else can feel free to do so.

Updated existing libraries-override CR https://www.drupal.org/node/2497313 to incorporate libraries-extend.

I also updated the CR to link to #2580255: Remove (classy|seven)_library_info_alter() in favor of libraries-extend, to point readers to the fact that Seven and Classy are now doing the right thing :)

ActiveTheme->librariesExtend was introduced here as an undeclared property.

Follow-up: #2959972: Undeclared property: ActiveTheme::$librariesExtend