Split drupal.js

I have doubts about this.

Anything using Drupal.t() would have to declare a dependency. This would be very useful for locale module actually, to know which files contain translations.

Let's see what nod_ thinks.

It would need to be Strings.t(), etc. then no? I know JS does not limit us to keep all Drupal.* in one library, but for consistency we are supposed to no?

I'm behind the split. How things are split can be discussed a tiny bit more.

For the naming: we can't use Strings.XXX that's much too close to the JS string object, that would be confusing on top of adding a new global that would confuse the hell out of non drupal people. As for naming it's pretty much free for all. We have mobile.install.js next to nav-tabs.js and there is no official relations between how the file is named and what's in there.

Moving string functions is good, but we still have api related functions next to behaviors stuff: url, theme, encode path, those manipulate strings as well. What could be a good split is to move initialization-related code and function in their own file like a drupal.init.js and leave the rest in drupal.js and add a dependency on drupal.init. We don't have to mess all the other libs that way. As for splitting translation related function (and not just string related) that could be good as wim points out.

At the very least, this needs work because we use 2 space indent, not 4.

Not sure this has something to do with D8MI at this point?

Thanks! Since that looks to be in accordance with the directives from our JS maintainer in #5: looking great :)

Still using 4-space indentation everywhere though.

+++ b/core/misc/drupal.js
@@ -1,16 +1,3 @@
-/**
- * Base framework for Drupal-specific JavaScript, behaviors, and settings.
- */

I think this should get a new @file header?

Can we have a change notice please.

.

All right had some time to think about it.

I'm not comfortable with the proposed split because we're splitting things that are related with things that are not. Maybe it's because of the naming of the files that it's a little confusing.

Here is my though process: drupal.js contains the Drupal API (not just "the rest" :p) and this API needs to be initialized, hence it depends on a drupal.init.js file that comes after drupal.js. We don't have to add more libraries, we can just say that the core/drupal lib is actually 2 js files. That way contrib can switch out dependencies on domready, mess with how things are initialized and we don't have to rename a huge amount of things.

1. we need to put window.Drupal = {behaviors: {}}; in drupal.js (can be inside the closure, no need to separate it).
2. Attach/DetachBehavior-related function should be back in drupal.js, it's part of the Drupal API after all.
3. content of drupal.domready.js should go in drupal.init.js, that init file would essentially contain the domready stuff, the js class stuff and the jQuery no conflict stuff.

Just one little comment:

+++ b/core/misc/drupal.js
@@ -1,16 +1,3 @@
 (function (domready, Drupal, drupalSettings, drupalTranslations) {

don't need domready here anymore.

Things should be clear in #17

per request from @penyaskito, canceled #20 since head had been failing, so the test was postponed, and there is a new one on this issue anyway.

I apologize about the "diet" patch name, it was a poor attempt at wit. https://the-pastry-box-project.net/marc-drummond/2015-december-21

It's over-optimizing. We can introduce a global var to on/off the domready init.

whatever, can't do it 8.0.x :)

Tried to test on simplytest.me against 8.3.x and patch failed. Will reroll

Adding rerolled patch for testing

Merge conflict in drupal.js

Tested on simplytest.me. Patch applied correctly.

Manually clicked around the install. Everything seems to load correctly and the attachBehaviors function is firing correctly. Checked that ajax on tags field was working and quick edit for an article. Ordering of files appears to be correct when combined.

Chrome and IE11

Updating tasks table in issue summary

Little nitpick,

drupal/core/misc/drupal.init.js
  14:3  error  Strings must use singlequote  quotes

✖ 1 problem (1 error, 0 warnings)

Can be fixed during commit I guess

This looks fine to me for 8.3.x, but needs a change record on the unlikely chance that someone is using library_override.

Also we should clarify the bc policy for location of js files - I think saying libraries_override is @internal would be fine, but don't think we say that yet.

That looks good to me. Thanks!

Everything still works.

testbot fail

Another random failure

@penyaskito libraries_override itself isn't @internal - we're not going to remove support for it in 8.x

However we can't guarantee that a specific file in a library is going to be override-able in the same way for the entirety of the 8.x cycle (especially not if we commit patches like this).

This is in the same way that hook_form_alter() is an API, but we specifically say the form array passed to the hook can and will change during the cycle.

Committed/pushed to 8.3.x, thanks!