Change JS settings into a separate asset type

Yes please.

I also get the array structure wrong for js settings wrong nearly every time I try to add them, and having to declare js settings in libraries doesn't work since js settings are nearly always dynamic.

Done, consensus built. I don't think anyone would lobby to keep the current DX.

One comment is that naming this $element[#attached][settings] might be confusing. Before you had the information that is was about JS. Some might end up thinking it's some kind of $form_state for render elements. Might not be an issue but from a distance it can look like that.

It's longer and somewhat uglier but how would you feel about: [#attached][drupalSettings].

I'll go with drupalSettings then, at least they have a clue where to find that stuff in their JS.

Not a problem for me. It's just an artifact of explicitness, in all the other places it'll look good.

FWIW, I also always get the syntax wrong, the first time I researched this for 2 hours until I found a good blog post that made it out as being a big secret ...

So BIG +1, my vote would be for drupalSettings as well.

The #attached key vs. hook name mismatch isn't great, but I also think drupalSettings and hook_js_settings_alter() are the best names individually, so fine with me.

RTBC, it would be great to show an example from the tests for hook_js_settings_alter() in the change record :).

$variables[#attached][drupalSettings] is so much win. Rtbc++

About the DX, when it's like this:
$element['#attached']['drupalSettings'][] = ['machineName' => …]

It's cool it makes sense. But when it's

'drupalSettings' => [[
        'batch' => [
          'errorMessage' => $current_set['error_message'] . '<br />' . $batch['error_message'],
          'initMessage' => $current_set['init_message'],
          'uri' => $url,
        ],
      ]],

It's kinda not obvious. I have an issue with the shorthand syntax for arrays so it might come from there. Conceptually though, while [library][] = ''; makes sense because it's a list of dependency, drupalSettings is less obvious. I don't see that we attach a list of settings but it's mainly about attaching a setting object and have all of them aggregated in the background.

Tried it out, it seems to work well (no idea about tests though). In common.inc around 1835:

  // Convert every JavaScript settings asset into a regular JavaScript asset.
  // @todo Clean this up in https://www.drupal.org/node/2382533
  if (isset($elements['#attached']['drupalSettings'])) {
    _drupal_add_js($elements['#attached']['drupalSettings'], ['type' => 'setting']);
    unset($elements['#attached']['drupalSettings']);
  }

I will never write this: 'drupalSettings' => [['foo' => TRUE], ['bar' => FALSE]]. I know it's example code but I expect(ed) the DX to be: 'drupalSettings' => ['foo' => TRUE, 'bar' => FALSE]
because that just makes more sense.

I'm for changing the API around this piece of cruft, we're already changing API anyway might as well make it as nice as possible.

Other than that, patch looks good.

I'm also tempted by the change to the array structure here, that's not any additional breakage for modules, and it'll be easier in the long run.

Great work, this looks really solid.

1. +++ b/core/lib/Drupal/Core/Installer/Form/SiteConfigureForm.php
   @@ -123,8 +123,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -    $js = array('copyFieldValue' => array('edit-site-mail' => array('edit-account-mail')));
   -    $form['#attached']['js'][] = array('data' => $js, 'type' => 'setting');
   +    $form['#attached']['drupalSettings']['copyFieldValue']['edit-site-mail'] = ['edit-account-mail'];
    
   

   This is lightyears easier to read.

2. +++ b/core/modules/history/history.module
   @@ -193,16 +193,7 @@ function history_user_delete($account) {
   +  $node_id = $context['node_id'];
   +  $element['#attached']['drupalSettings']['history']['lastReadTimestamps'][$node_id] = (int) history_read($context['node_id']);
   

   Nitpick: Now that you have extracted the $node_id into a variable you can also use it on the right hand side.

3. +++ b/core/modules/system/src/Tests/Common/MergeAttachmentsTest.php
   @@ -202,48 +198,95 @@ function testJsSettingMerging() {
   +    $expected_settings_two['moduleName']['magical flag'] = 3.14;
   

   Just in case you ever need a more precice number, you can also call pi() or use M_PI :)

Reviewed the patch tested the UI and everything looks good. The DX is so nice to work with.

The only detail I was unsure of is the fact that drupalSettings get it's default values in system_js_settings_alter, but we get some of the drupalSettings values in the library file so it helps with discovery. Anyhow I never had to use js settings value from PHP before so I don't feel like setting those values a bit later is any problem.

RTBC for me, let's finish this :)

I think there is some documentation that needs to be updated here, some in the patch and some on drupal.org documentation pages, right?

So we need to make sure all topics and other documentation on api.drupal.org that references JS settings is updated for this change, as part of the patch. And in the issue summary, we need to add a section that lists pages on drupal.org that will need to be updated if this patch goes in.

Also before this issue can be marked RTBC it needs a "Beta Evalualuation" section added to the summary. See https://www.drupal.org/contribute/core/beta-changes

Places we may need docs updates on api.drupal.org:
https://api.drupal.org/api/drupal/core!modules!system!theme.api.php/grou...
https://api.drupal.org/api/drupal/core!includes!common.inc/function/drup...

We shouldn't have updated the drupal.org pages until *after* the patch went in... oh well, hopefully it will go in.

Great move !

Not a fan of the 'drupalSettings' key, that is not explicitly related to JS and which, seen in PHP code, doesn't reflect what the code does - pass stuff for the JS side. ["#attached']['js_settings'] would be more telling IMO.

IMHO drupalSettings key is consistent with the name of the core library, with the global variable window.drupalSettings, and it's widely used in all core javascript files. It makes sense fast, once that people start to work with D8.
The lower camel case format used, and being inside ['#attached'] already hints you that it must be JavaScript settings.
It's not totally clear, I agree, but better to be consistent than adding a new exception.

The *lower camel case* format used, and being inside ['#attached'] already hints you that it must be JavaScript settings

camelCase is used in lots of non-JS contexts in Drupal 8, so camelCase is no hint for JS. Neither is #attached ?

But well, I see the patch also changes the 'settings' key in *.libraries.yml entries to 'drupalSettings'. Even there, it's not fully clear that those end up in JS, but well, consistency++.

Would be clearer IMO if window.drupalSettings was a jsSettings entry in the existing Drupal object, that way all code referring to 'jsSettings', including on the PHP side or in YAML files, would be pretty self-explanatory. Much larger change, though.

[edit: also, would keep hook_js_settings_alter() in touch with what it actually works on]

In short, still not fully convinced, but I'll shut up if it's just me :-)

We have to split drupalSettings and Drupal object because we want settings values available ASAP, we don't want settings depending on either jQuery or drupal.js and sometimes we want the JS settings but not the drupal.js file. See JavaScript settings moved from Drupal.settings to global drupalSettings variable.

And copying drupalSettings in Drupal.settings is just asking for trouble :p

@yched you have a point. My argument there was invalid. I made a regex search \["[a-z]+[A-Z0-9][a-z0-9]+[A-Za-z0-9]* in D8 code base and there is many times (about 50% of the times) where camelCase associative keys are used, and it's not related to JS.

Still, I will go for consistency, drupalSettings is quite memorable and easier to identify as global object in the JS side. Not so much in PHP/YAML side true.

[edit: crosspost with #54]

@nod_: OK, thanks for the explanation.

The hiatus of "the hook to alter 'drupalSettings' is hook_js_settings_alter()", which is the only name that makes sense for that hook, is IMO exactly symptomatic of the fact that from the POV of PHP code, what you do is "place settings for the JS side", not just "place settings" or "place Drupal settings".

So ideally we'd need something that :
- on the JS side, is namespaced to "drupal" somehow, but cannot be in the Drupal global
- on the PHP side, is namespaced with "js" somehow, since that's how PHP code sees it.

Just trying to reformulate the problem space in case this sparkles something with someone, but yeah, atm I must confess I have no better suggestion than the current patch...

FWIW, not trying to bikeshed, but I agree with @yched in #49 and #52. It's just for the record though and shouldn't hold up this patch.

Great work @WimLeers.

WimLeers++

#55: How about drupalJsSettings?

@Wim : sure, I totally see the other consistency benefits, no questions here :-)

drupalJsSettings / hook_drupal_js_settings_alter() - yeah, wouldn't that work ?

Having a JS variable say "I'm about JS" might seem redundant, but IMO it's in fact accurate, if you consider that Drupal is a PHP-first tool, and is thus completely entitled to have a "component for passing settings to the JS side, that is implemented by code in both PHP and JS working hand in hand". When seen from JS code, well yeah, it's the JS part of "the drupal system handling JS settings".

@Wim I think yched means using drupalJsSettings both in PHP and JavaScript - so renaming drupalSettings.

That's even more of an API change here - but also JavaScript is far from frozen at this point and any module setting it's own settings will have to update after this lands anyway.

I don't have a strong opinion on the naming. I do agree that drupalSettings is definitely the least worst option until #59 so would be fine doing that if we eventually decide on it, and also think it's worth discussing this a bit longer so we don't regret missing an option later.

The constraints on the JS side are:

1. Not in the global Drupal object (or we'd need to change what the Drupal is, not a fan).
2. Has to be obvious the object is not part of some other JS library and won't conflict. Some have messed up names so it can be hard to find
3. Ideally, has to be recognizable as coming from Drupal (with drupal in the name somewhere)

drupalSettings isn't great but to me better than backendJsSettings, drupalJsSettings is even longer and uglier with a capitalization on th "JS" that kinda make my eyes bleed. But if consistency win, so be it. It all get minified anyway.

That's a really nice change and as a themer I can say this definitely improves the experience. Adding JS settings has been pita since forever. I think this doesn't only make it easier to use but also easier to understand. Also hook_js_settings_alter() seems to be a good improvement at least from my perspective.

I'm not 100% happy with the name drupalSettings because it's not the most descriptive. However I couldn't find or figure out anything better for this so I don't have any suggestions.

Good job everyone!

There is no much freedom to choose names. Although it's verbose, I think drupalJsSettings is accurate and explicit. It:
* Must contain word Drupal (nod_ #62)
* Must contain a reference to JavaScript or JS
* will be nice that is "settings",(no 'config' for example) because then will make sense when using hook_js_settings_alter ( yched #52 )
So drupalJsSettings seems to be the only candidate that ticks all the boxes. Or jsDrupalSettings (ugly)

Repeating the code from Wim at #57:

function fluffiness_page_attachments_alter(&$page) {
  $page['#attached']['library'][] = 'fluffiness/cuddly-slider';
  $page['#attached']['drupalJsSettings']['fluffiness']['cuddlySlider']['foo'] = 'bar';
}

In JS:
drupalJsSettings.flufiness.cuddlySlider.foo

+1 to making it a separate asset type, I've felt the pain as well. I like that the proposed name (drupalSettings) is the same on both ends.

To me the greater problem with regards to naming may actually be with render arrays and the difficulty documenting them. If there was a standard way to document the possible sub-keys of #attached then to me the array key name on the PHP side is much less of a concern, as long as the asset types are easy to look up.

What about:

  $page['#attached']['js.drupalSettings']['fluffiness']['cuddlySlider']['foo'] = 'bar';

window.drupalSettings.flufiness.cuddlySlider.foo

I think with the prefix "js." we do not confuse too much, say it is JS related and still keep the namespace pretty clear.

hook_js_settings_alter() could indeed be hook_js_drupal_settings_alter() then and it would all feel pretty good.

re @Wim #60

drupalJsSettings in PHP <> drupalSettings in JS.

Right, no, it has to be the same name on both sides of course. i think drupalJsSettings works.

perhaps all of this points to the fact that "settings" is actually a misnomer. Often, the data in drupalSettings are not so much settings as it's state data

Agreed that it's about "data" more than "settings", but yeah, I don't think that changes much on the current naming issue.

But no matter what, I think you'd always need to clarify on client side that it's coming from the server and on the server you'd need to clarify that it's for the client

Yep, it's a mirror :-)
But my reasoning with drupalJsSettings working on both sides is that :
- it is more important to disambiguate "this is about JS" on the PHP side, as hook_js_settings_alter() shows. Drupal is primarily about PHP, so unless somthing explicitly states it's about JS, then you assume it's about PHP - which is why hook_drupal_settings_alter() wouldn't work.
- On the JS side, "this comes from the server" is somewhat carried by "drupal" : in drupal, things come from php on the server, where would they come from otherwise ? (well, local storage I guess - but we don't have a specific drupal API/layer for that, right ?)
In other words, much less ambiguity to worry about on the JS side IMO.

+1 to this improvement.

Reg: drupalSettings name - I don't really mind *the name* considering what we have now and surely we won't regret in future (touch wood) because of this name :)

OK I thought about this a bit more and discussed briefly with Alex Pott and Wim Leers in irc.

- drupalSettings we've already changed and it's close to Drupal.settings in 7.x. Existing JavaScript that already uses settings provided by core won't need to update if we keep it the same (probably a tiny number either way though).
- #attached will only have library and drupalSettings by the time we're done here, that's only two things to figure out so it's already much more learnable than Drupal 7. The fact it's in #attached at all makes it obvious it's a front end thing (once you know what #attached itself is for).
- I like the consistency of drupalJsSettings between js and PHP, but it comes at the cost of redundancy in JavaScript. Also visually the lowercase l and upper case J make it hard to look at and have me second guessing the spelling for typos. nod_ alluded to a similar thing.
- that leaves hook_js_settings_alter() in the cold a bit, but hook_js_settings_alter() doesn't match drupalJsSettings in terms of googling/grepping anyway, and it's a rarely used hook.

So... I'm going to go ahead and commit this as is. Committed/pushed to 8.0.x, thanks!

Fine with the decision, doesn't reduce the awesomeness of this patch anyway :-)
@Wim++

Great!
That was a sensitive balance and decision.
Wim++

This seems to have caused #2388215: Tests for Drag and drop

#74 is same as #2387523: Tabledrag broken after JS setting separate asset type

Is this issue the root cause of #2391025: Add support for inline JS/CSS with #attached?

@hass yep, it is Working as designed (WAD). Check the change notice for more details.

It's a design flaw and these change notice does not document what I'm doing.