Attach assets only via the asset library system

Agreed on removing inline js. When I've done site audits a very common problem is a poorly written contrib or custom module sticking inline js on the page instead of using Drupal.settings. Then that inline js because it is added in amongst all the other js on the page results in two aggregates (one before the inline js, one after) where there originally would have been one. So supporting that as a regular asset type makes it very easy to mess up the front end performance of a site.

I wonder a little bit about inline CSS - some rendering strategies try to group layout CSS inline so that it gets downloaded very, very quickly as part of the initial HTML request. I guess that's something that would happen in the css aggregation/rendering stages anyway though and no way a module can do this in #attached anyway.

I agree that inline css/js messes up aggregates, but this can be fixed; in advagg there is a setting that forces all inline code to be at the bottom, there is another setting that forces all external code to be at the top, and a 3rd setting that will group all browser conditionals together; these are all things that can mess up aggregates. Having a standard way to add in inline code is nice as it allows one an easy way to modify it; be it minification or delayed execution or modification of the code if it was added by a module.

+++ b/core/modules/history/history.module
@@ -135,14 +135,11 @@ function history_cron() {
   if (($display->originalMode === 'full') && \Drupal::currentUser()->isAuthenticated()) {
-    // When the window's "load" event is triggered, mark the node as read.
-    // This still allows for Drupal behaviors (which are triggered on the
-    // "DOMContentReady" event) to add "new" and "updated" indicators.
-    $build['#attached']['js'][] = array(
-      'data' => 'window.addEventListener("load",function(){Drupal.history.markAsRead(' . $node->id() . ');},false);',
-      'type' => 'inline',
+    // Embed the metadata for the "X new comments" link (if any) on this
+    // entity.
+    $build['#post_render_cache']['history_attach_timestamp'] = array(
+      array('node_id' => $node->id()),
     );
-    $build['#attached']['library'][] = 'history/drupal.history';
   }

This is not correct, this has to be triggered by javascript.

The point of doing it in javascript is that it also works when delivering pages from page page/varnish.

In common.inc there is still

  // Convert every JavaScript settings asset into a regular JavaScript asset.
  // @todo Clean this up in https://www.drupal.org/node/2382533

What's the plan for that clean-up?

in _drupal_add_library:

          // If the value is not an array, it's a filename and passed as first
          // (and only) argument.
          if (!is_array($options)) {
            $data = $options;
            $options = NULL;
          }
          // In some cases, the first parameter ($data) is an array. Arrays can't be
          // passed as keys in PHP, so we have to get $data from the value array.
          if (is_numeric($data)) {
            $data = $options['data'];
            unset($options['data']);
          }

Seems to me that $data will always be numeric and $options will always be an array because we declare everything through libraries and in the YML, js and css are always objects, not a mix of [filename, filename] and {filename: options, filename: options}:

  js: 
    somefilename.js: {}

Tried it and looks like the following is enough:

      // Add both the JavaScript and the CSS.
      // The parameters for _drupal_add_js() and _drupal_add_css() require special
      // handling.
      foreach (array('js', 'css') as $type) {
        foreach ($library[$type] as $data => $options) {
          call_user_func('_drupal_add_' . $type, $options['data'], $options);
        }
        unset($elements['#attached'][$type]);

in JsCollectionRenderer

    // For inline JavaScript to validate as XHTML, all JavaScript containing
    // XHTML needs to be wrapped in CDATA. To make that backwards compatible
    // with HTML 4, we need to comment out the CDATA-tag.
    $embed_prefix = "\n<!--//--><![CDATA[//><!--\n";
    $embed_suffix = "\n//--><!]]>\n";

I so want those gone, but looks like we still "need" it for drupalSettings. Out of scope though.

In History module
$build['#attached']['drupalSettings']['history']['nodesToMarkAsRead'][$node->id()] = TRUE;
can we have
$build['#attached']['drupalSettings']['history']['nodesToMarkAsRead'][] = $node->id();
Instead? It's the ajaxPageState situation again, are we really worried about duplicates here?

Looks good to me otherwise :)

This patch looks really great. It reduces a lot the amount of different ways how to do things: 'css' vs 'library', 'library' vs. 'inline' ...

+++ b/core/includes/common.inc
@@ -1305,10 +1285,6 @@ function drupal_clean_id_identifier($id) {
  *   _drupal_add_js(array('myModule' => array('key' => 'value')), 'setting');

@@ -1694,44 +1662,40 @@ function drupal_merge_attached(array $a, array $b) {
+ * - 'drupalSettings' (JavaScript settings)

Just curious, is this example still valid?

Better tests, better DX, everything still working. No more complains.

Basically the patch changes history module to not use inline JS, do the API removal change and do lots of test improvments.

Only had nitpicks, fixed on commit:

1. +++ b/core/includes/common.inc
   @@ -1350,11 +1324,11 @@ function drupal_clean_id_identifier($id) {
   + *   the $data parameter ('file'/'setting'/'external'), or an assocative array.
   

   associative

2. +++ b/core/includes/common.inc
   @@ -1694,44 +1662,40 @@ function drupal_merge_attached(array $a, array $b) {
   + * is an associative array, where the keys are the the attachment types and the
   

   Not introduced by the patch, just moved around, but 'the the'.

Everything else looks great. Simplifies the system itself and makes things more consistent for themes and modules. Also unblocks both the AJAX pagestate optimization and doing asset generation in separate http requests.

Committed/pushed to 8.0.x, thanks!

How do I add a dynamically generated stylesheet that could have any name?

One of my modules and a theme can build stylesheets - there is no library. I can never know the name of these files in advance, it depends on the user input.

User saves a form, file is generated and saved to the file system. I save the path/name of file etc and load it at the appropriate time.

How can I load these files?

Jeff, you will need to manage libraries for each file

If there's no library for the file to go in, then a completely new library could be added dynamically once #2358981: Provide a mechanism for dynamic library declarations is in.

Just be aware of this #2050269: hook_library_info_alter() is not called for themes

I'm going to have a crack at solving my issue with hook_css_alter(), since that is probably the only real option I have for now.

What we really need is #2358981: Provide a mechanism for dynamic library declarations

I think this change record will have to be updated: drupal_add_css(), drupal_add_js() and drupal_add_library() removed in favor of #attached

Perhaps others as well.

+++ b/core/modules/comment/comment.libraries.yml
@@ -30,7 +30,7 @@ drupal.comment-new-indicator:
-    - history/drupal.history
+    - history/api

comment does not depends on history. is this fine?

From case intro this issue may the the root cause of #2391025: Add support for inline JS/CSS with #attached

I read Remove support for inline CSS and JS assets (see #5.3 + #8 for details). what is a clear failure!

Heres one of my cases that is, as I see it, very difficult to work around since this change.

I used inline JS for live reload, Typekit fonts and other things, this change forced me to use the asynchronous Typekit method which always gives a FOUT and is pretty dam hard to avoid even using their state classes. Inlining the JS is very fast method for Typekit, same goes for Google fonts, if you inline the link its super fast, but I cannot do that now (users select their font in settings), and I have to use their asynchronous JS method which gives a FOUT most of the time and there are no state classes at all.

I'm looking for ways around these issues, I understand the concerns in the issue and there maybe ways to get around the FOUT's etc, but this has made things much more difficult for front enders.

See the 'file-inline' type that we could use to fix this. Need to open an issue about it.

What is 'file-inline' ? I need full inline code without files. The inline code I'm adding may dynamic per user, too.

Sorry was on my phone couldn't find the issue. What I mean is the "inline js" part of this comment #2298551-9: Documentation updates adding/adjusting inline/external JS in themes.

I'm replying in the other issue. I don't see a reason to go back on the principle of this issue. Inline scripts are not considered assets anymore, it's just random HTML bits, that can be added by #markup in the body of the page or with #attached => html_head.

I read a lot regarding the latest changes to js/css libraries here, and in all the related issues. I still can't find a solution for my problem. Consider this - I use a field formatter/widget for displaying/editing a map (Google/OSM). This map is displayed on every node. All the js required for the map itself is defined in my module.libraries.yml (igwmap.formatter.js), easy and clear. For the map layers, for example an GPX track overlay, all the required js is stored in a file during node save, so each single nid has a separate js file to load. For now I did this (in FieldFormatter):

      $elements[$delta] = [
        '#markup' => $map_div,
        '#attached' => [
          'library' => ['igwmap/igwmap.formatter.js'],
          'js' => [
            [
              'type' => 'file',
              'data' => $item->static_js,
              'scope' => 'footer',
              'weight' => 1,
              'defer' => FALSE
            ]
          ]
        ],
      ];

where $item->static_js is the full path to the js file for the current node to load, so something like this files/tracks/js/node_1234.js. I do not see how to work around this by the provided examples, so this seems like a regression to me. Also non of the hooks_library_something() seem to do the job. I see why we need these changes and they all make sense to me, but leave me with a problem I have no idea how to get around. Any suggestions welcome :-)

Thanks @Wim! I did it some other way, but you pointed me to the right direction :-)

making tag the more common one so we can delete the lesser used one so it does not show in the autocomplete