This would mean outputting the settings before any other JS tag is added, since it's the first javascript it will never need to use $.extend (the jQuery dependency) and a good DX thing is that settings would be available right away in the JS files, no need to wait for attach behaviors to be run to access them.

The drupal.js dependency is another subject, do we want the settings to create the global [D|d]rupal object, or do we want to create it anyway, all the time, even if it's empty? I guess this bit is up for discussion, we really need to find a way to remove the $.extend thing anyway.

Change record

Drupal.settings changed to drupalSettings:
In order to reduce the dependency on jQuery (and in particular the need for deep object merging like with $.extend), the Drupal.settings object was given its own top-level namespace, namely drupalSettings. For legacy reasons, the drupalSettings object is still copied over to Drupal.settings, but the settings are no longer merged by means of $.extend.

Files: 
CommentFileSizeAuthor
#37 drupal8.js-settings.37.patch15.76 KBsun
PASSED: [[SimpleTest]]: [MySQL] 41,484 pass(es).
[ View ]
#34 drupal8.js-settings.34.patch15.78 KBsun
FAILED: [[SimpleTest]]: [MySQL] 41,383 pass(es), 102 fail(s), and 105 exception(s).
[ View ]
#24 core-js-remove-jQuery-settings-1760548-24.patch17.26 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 41,480 pass(es).
[ View ]
#22 core-js-remove-jQuery-settings-1760548-22.patch17.25 KBnod_
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-remove-jQuery-settings-1760548-22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#15 core-js-remove-jQuery-settings-1760548-15.patch17.35 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 41,293 pass(es).
[ View ]
#7 core-js-remove-jQuery-settings-1760548-7.patch6.71 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 40,704 pass(es).
[ View ]
#5 core-js-remove-jQuery-settings-1760548-5.patch4.42 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 40,610 pass(es), 102 fail(s), and 105 exception(s).
[ View ]
#3 core-js-remove-jQuery-settings-1760548-3.patch2.66 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 40,437 pass(es), 105 fail(s), and 105 exception(s).
[ View ]

Comments

nod_’s picture

Status:Postponed» Active

Patch is in :)

seutje’s picture

Personally, I never understood why Core was using $.extend, as it will always be the first to define settings. Perhaps we can have dynamically loaded settings to define them in a way that doesn't require $.extend, but does that mean all settings should be defined in this way?

nod_’s picture

Status:Active» Needs review
StatusFileSize
new2.66 KB
FAILED: [[SimpleTest]]: [MySQL] 40,437 pass(es), 105 fail(s), and 105 exception(s).
[ View ]

Removing jQuery and Drupal. This include the patch from here:#1764634: Drupal.settings is only populated if a custom setting is added.

Status:Needs review» Needs work

The last submitted patch, core-js-remove-jQuery-settings-1760548-3.patch, failed testing.

nod_’s picture

Status:Needs work» Needs review
StatusFileSize
new4.42 KB
FAILED: [[SimpleTest]]: [MySQL] 40,610 pass(es), 102 fail(s), and 105 exception(s).
[ View ]

Other patch got in, reroll, less tests failures hopefully.

Status:Needs review» Needs work

The last submitted patch, core-js-remove-jQuery-settings-1760548-5.patch, failed testing.

nod_’s picture

Status:Needs work» Needs review
StatusFileSize
new6.71 KB
PASSED: [[SimpleTest]]: [MySQL] 40,704 pass(es).
[ View ]

better?

nod_’s picture

Ok so the problem was because of the regex with which simpletest grabs the settings from a page. All good :)

nod_’s picture

Title:Remove dependency on jQuery (and drupal.js?) for JS settings» Remove dependency on jQuery drupal.js? for JS settings
seutje’s picture

Status:Needs review» Needs work
+++ b/core/misc/drupal.jsundefined
@@ -1,14 +1,17 @@
+// Populate Drupal.settings with the drupalSettings variable.
+Drupal.settings = drupalSettings;

I'm confused, why is this still being copied over to Drupal.settings, or why does it seem like everything is using drupalSettings anyway? seems like one of these 2 should be irrelevant...

nod_’s picture

Kept that around to not break contrib horribly. I'm fine if we get rid of it.

kristiaanvandeneynde’s picture

That's what change notifications are for.
Let's keep confusing code out of Drupal 8 :)

seutje’s picture

ah, that was my best guess. If we keep it around, wouldn't that sorta defeat the purpose? And it should probably get an explanatory comment line or something.

seutje’s picture

Discard my comment about "defeats the purpose", was being silly, as usual.

nod_’s picture

Status:Needs work» Needs review
StatusFileSize
new17.35 KB
PASSED: [[SimpleTest]]: [MySQL] 41,293 pass(es).
[ View ]

Removing Drupal.settings is fine but it's making a big patch in a bigger patch.

Tha'ts useful on it's own and removing Drupal.settings can be a follow-up.

I actually need this patch to make nice and helpful perf measurements for some benchmark of js changes.

New patch renamed the library from drupal.settings to drupalSettings since it's not part of the Drupal object anymore (and yes, drupal. and not Drupal. in library names since that'll be renamed soon enough too).

seutje’s picture

I see where you're getting at, I'm already very glad to get rid of this awkward extend.

I suppose this needs a change notification though?

nod_’s picture

Yep. Want to take care of it?

There are simpletests for this, no need to have js testing.

seutje’s picture

Status:Needs review» Reviewed & tested by the community

Sure, I just add a record on http://drupal.org/list-changes/drupal right?

But I guess it needs to be committed first, no?

nod_’s picture

Yep that's the process. catch told me it's better to make them after commit :)

seutje’s picture

Issue summary:View changes

Added change record information to the summary.

seutje’s picture

Added change record to the issue summary.

catch’s picture

Status:Reviewed & tested by the community» Needs work

This no longer applies, quick re-roll?

nod_’s picture

Status:Needs work» Needs review
StatusFileSize
new17.25 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-remove-jQuery-settings-1760548-22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

was broken by the remove type="text/javascript" thing.

Status:Needs review» Needs work

The last submitted patch, core-js-remove-jQuery-settings-1760548-22.patch, failed testing.

nod_’s picture

Status:Needs work» Needs review
StatusFileSize
new17.26 KB
PASSED: [[SimpleTest]]: [MySQL] 41,480 pass(es).
[ View ]

Sorry made a diff from the wrong branch.

seutje’s picture

Status:Needs review» Reviewed & tested by the community

Looks good to me!

catch’s picture

Title:Remove dependency on jQuery drupal.js? for JS settings» Change notice for: Remove dependency on jQuery drupal.js? for JS settings
Priority:Normal» Critical
Status:Reviewed & tested by the community» Active

Thanks! Committed/pushed to 8.x.

nod_’s picture

Title:Change notice for: Remove dependency on jQuery drupal.js? for JS settings» Remove dependency on jQuery and drupal.js for JS settings
Priority:Critical» Normal
Status:Active» Fixed
Wim Leers’s picture

How are we now then supposed to deal with dynamically loaded settings? (e.g. an AJAX command adds some HTML which comes with an attached JS behavior, which comes with its own settings.) AFAICT, it continues to work the same way, i.e. it still uses $.extend, it's just that we don't need it anymore for the *initial* Drupal JS settings?

nod_’s picture

Yes that's right. Since we're still keeping settings in Drupal.settings around to not break everything, it'll just work.

drupalSettings will always be present when using the ajax commands since ajax.js is depends on the settings.

Wim Leers’s picture

Okay. We have a change record, which links here, and the question has been answered here. That's good :)

*However*, ajax.js still updates Drupal.settings, not drupalSettings. Shouldn't we try to get everything to move over to use drupalSettings? That will be addressed in follow-up issues, I presume?

nod_’s picture

Yes I'll be a long patch to make, I'd rather have that taken care of after feature freeze. Here is the follow-up #1793648: Follow-up: replace all occurence of Drupal.settings with drupalSettings.

Wim Leers’s picture

Agreed! :)

sun’s picture

I don't quite get why we had to introduce a new global, instead of simply declaring Drupal.settings without jQuery?

<script>
var Drupal = Drupal || {};
Drupal.settings = Drupal.settings || {.......};
</script>
sun’s picture

Status:Fixed» Needs review
StatusFileSize
new15.78 KB
FAILED: [[SimpleTest]]: [MySQL] 41,383 pass(es), 102 fail(s), and 105 exception(s).
[ View ]

There.

sun’s picture

Priority:Normal» Major
Issue tags:+API change

It's also noteworthy that this overall change (regardless of #34) has a few implications:

1) Settings MUST NOT be output twice on the page; e.g., 'scope' => 'footer'. The former $.extend() took care of properly merging multiple instances. The current JS processing and rendering for JS settings does not take that into account.

2) Likewise, an Ajax request MUST NOT return bare/plain drupalSettings/Drupal.settings, but only ever Ajax settings commands. The current Ajax framework does not ensure that.

3) There is generally no way to extend or enhance settings after Drupal settings have been rendered/output. This was previously possible through inline JS and other means.

nod_’s picture

Priority:Major» Normal
Issue tags:-API change

because in every library out there settings are not part of the library namespace. you do get defaults but what's in drupalSettings are hardly defaults at the library level (you rightly made me take it out of PHP libraries definitions after all :)

once you get amd and this kind of things (even jut with our closure) it just doesn't matter anymore. i can't see how we'd run into a naming conflict either.

so yeah settings are just not library related stuff and should be separated.

(edit) sorry for status change. writing from phone takes ages.

sun’s picture

Priority:Normal» Major
Issue tags:+API change
StatusFileSize
new15.76 KB
PASSED: [[SimpleTest]]: [MySQL] 41,484 pass(es).
[ View ]

I'm sorry, but that terribly sounds like a second-guessed reasoning ;)

If we get to that level, then I will happily hold against that: Polluting the global namespace with variables is bad practice.

The original intention and scope of this issue was to remove the dependency on jQuery and drupal.js for JS settings.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -2019,7 +2019,7 @@ protected function drupalSetContent($content, $url = 'internal:') {
+    if (preg_match('/jQuery\.extend\(Drupal\.settings, (.*?)\);/', $content, $matches)) {
       $this->drupalSettings = drupal_json_decode($matches[1]);

Sorry, forgot this one.

nod_’s picture

I mean you obviously were not convinced from the other explanation so I tried another one.

Let's be practical.

1) Actually you can put scope footer for settings, it just doesn't work. Try it. An issue in and of itself but not an issue here.

2) How does using Drupal.settings prevent Ajax requests to override the whole array? if you're using ajax outside of the Ajax framework you take care of this. We don't babysit broken code right?

3) You can still do: drupalSettings.myCustomSetting = {};, it's just that by default you don't get $.extend() but that's unrelated to the drupalSettings/Drupal.settings thing.

sun’s picture

Righto, as I already mentioned, none of the issues listed in #34 are about drupalSettings vs. Drupal.settings, because they share the same semantics, since only the variable name differs.

What strikes me with drupalSettings is that 1) it looks awkward both in PHP and JS, 2) it pollutes the global namespace for no good reason, 3) it was and is unnecessary for the actual desired change, as the patch proves.

nod_’s picture

I'm interested to see what my co-maintainers think about this issue :)

seutje’s picture

To be honest, I don't care where it lives right now. Until the Drupal namespace looks more like a library, it doesn't really matter all that much. We will probably need to do this at some point, though.

jessebeach’s picture

I see this patch as a stone in the path to a larger architectural change, namely an AMD structure. The details might change in the coming months but it's a good first step. My comments are really more for us to consider in those changes we will continue to make.

I'd really love to see us using an extend pattern here. It's far more resilient. We'll need some basic utilities that are available cross-module e.g. extend(). We can't expect every module to implement their own utils. One of the value propositions of using Drupal is that lots of basic, foundational code has been written for you and is available to leverage. Is the plan to require modules to require the drupal.js file or does drupal.js just disappear? Do we plan to load no JavaScript file in vanilla core that contains common utilities but instead repeatedly reimplement these utils in each module or misc JS file as needed? I can think of a few that would be nice to have

  • extend
  • clone
  • debounce
  • map

Certainly there are more. It would probably be better to just to use underscore really.

I understand the intent to rid ourselves of any Drupal JS object in the global namespace. But if drupalSettings is really unique to its context, why not just call it settings? As @sun mentions in #39, the semantics are the same in the current drupalSettings vs. Drupal.settings. Do we need a pseudo-namespaced variable for backwards compatibility?

sun’s picture

But if drupalSettings is really unique to its context, why not just call it settings?

I have troubles to understand what you mean, but if you mean a global window.settings object with this, then that won't fly at all, since it breaks all concepts of embeddable applications that exist. The rule of thumb is to presume that Drupal is not the only application that is involved in producing a final page output (neither on the PHP side, nor on the JS side). Thus, there can be a poorly architected app/script next to Drupal that expects its options in window.settings, but 1) Drupal is not poorly designed, and 2) Drupal is able to co-exist and play nice with other libraries by properly namespacing its own stuff.

(Admittedly, the PHP side of things is not that clean yet, so it's only by coincidence that you may be able to embed Drupal into another PHP application. But that's a major bug, and not to be understood for a given.)

nod_’s picture

So obviously you're not against the conceptual change provided what you're raising in #35 doesn't happen to be an issue (I think it's not but I'm happy to be shown counter-examples of how it can break things for contrib/themes).

Then there is the naming. Introducing a global variable is "bad". Well, I think you brilliantly demonstrated in #1791872: [Policy, no patch] Rethink Drupal core's code quality review process that we tend to be too pedantic. JS community is not immune, it used to be so bad we had incredibly strict rules set up. Making this change, we know what we're doing we have documentation for it and all that. I mean I took out most/all leaking variables from core, this is different than a script that is leaking a "i" variable that might get messed up. Who would be using the drupalSettings variable out there?

And about the Drupal = Drupal || {}; Drupal.settings = Drupal.settings || {}; In which case can this ever use Drupal.settings and not the fallback? Settings are added way before everything else (the weight is -200) and if relying on weight is not enough, we now have explicit dependencies for everything, we can set the order based on the dependencies and _that_ will make sure that drupalSettings is always first, not needing the || pattern. Unless someone knowingly remove the dependency on settings and do something before, but I don't see how that'd be our problem.

This kind of change is also helping reduce the barrier of entry, there are decent JS coders out there (I knew a few) that didn't know about the || thing. Since it's essentially a shortcut for if and they were groomed by jslint putting ifs and brackets everywhere. With this construct you really can't guess what it does when you don't know, how is that not supposed to return a boolean? Variable + JSON object, they'd know.

Not mentioning how ugly the construct is. If you're saying we should have $.extend() in drupal.js to keep the previous behavior, that'd be fair enough and I don't see an issue with that. The new drupalSettings variable doesn't strike me as bad construct or poorly designed code. It's just more straightforward and straightforward is good (and faster). just using "settings" is kinda dangerous though, I agree it's likely to make problems. We're not at the AMD step just yet.

@jesse: no we're not making the same functions all the time. underscore is neat but monolithic, it's like a small jQuery for utilities. Mark pointed me to http://lodash.com/ which looks great for what I want to have possible. I guess we need a dedicated issue about all this.

(edit) ok just realized you don't have the || thing in your patch. (edit2) actually there is. Sleep…

nod_’s picture

Status:Needs review» Fixed

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

nod_’s picture

Fun fact, the new twitter blog has code to make Drupal.settings available on page load before behaviors are run. Not really important but interesting.

nod_’s picture

Issue summary:View changes

Updated issue summary. Added id to the "change record" heading for linking purposes.