In #1149866: Add Backbone.js and Underscore.js to core, the Backbone and Underscore libraries were added to Drupal with references to the uncompressed development versions. Before the release of Drupal 8, we need to switch the references to the minified and compressed production versions of these libraries.

Optionally update to the latest releases as well. Backbone should have a 1.0 release before D8 is released and it would be great to ship with this stable version.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jessebeach’s picture

Status: Postponed » Active
Issue tags: +JavaScript

Backbone now has a 1.0 release.

http://backbonejs.org/

We are approaching code freeze. I'm de-postponing this issue and marking it active. We need to get Backbone upgraded and the production version in place before code freeze.

JohnAlbin’s picture

Status: Active » Needs review
FileSize
318.94 KB

We MUST get rid of core/misc. I'm moving 3rd party assets over to core/assets/vendor in #1663622: Change directory structure for JavaScript files

Here's a patch that upgrades backbone.js to 1.0.0 and uses the backbone.min.js in the library definition. It also uses a .gitignore file we have for ckeditor to strip out the docs, examples and tests out of the backbone 1.0.0 downloadable distribution.

JohnAlbin’s picture

Forgot to update the .jshintignore file

Status: Needs review » Needs work

The last submitted patch, 1800022-3-backbone-upgrade.patch, failed testing.

JohnAlbin’s picture

Status: Needs work » Needs review

Fatal error in Drupal\rest\Tests\Views\StyleSerializerTest->testPreview()

Huh? Retesting.

JohnAlbin’s picture

#3: 1800022-3-backbone-upgrade.patch queued for re-testing.

Wim Leers’s picture

@JohnAlbin: you can't just upgrade Backbone; you'll have to update all JS in core to make it compatible. Most will be easy, but Edit's won't be. Also, this should wait for #1678002: Edit should provide a usable entity-level toolbar for saving fields.
I agree we must upgrade to 1.0 of course :)

JohnAlbin’s picture

you'll have to update all JS in core to make it compatible

Ahhh… oh, well. :-)

Status: Needs review » Needs work
Issue tags: -JavaScript, -revisit before beta

The last submitted patch, 1800022-3-backbone-upgrade.patch, failed testing.

nod_’s picture

Title: Swap the development versions of Backbone and Underscore for the production version » Update Backbone and Underscore
Priority: Normal » Critical
Issue summary: View changes
jessebeach’s picture

I won't be able to get to this in the coming week.

I've been backporting Toolbar to Navbar, which is running on Backbone 1.0. So far I've only found one upgrade issue: #2139571: underscore.js fails to compare DOM nodes correctly; Toolbar JS will fail when upgraded to Backbone 1.0.0.

wimleers is currently backporting D8 Edit to D7. Those two modules are the ones using Backbone most extensively. Once he's had a chance to do that backport, we'll know what issues we'll have with an upgrade.

Wim Leers’s picture

Status: Needs work » Postponed
Issue tags: +JavaScript, +Spark, +sprint
FileSize
150.81 KB
38.09 KB

Only in contextual.module and edit.module, invasive changes were necessary. Contextual.module should be tested very thoroughly. Edit.module too, but to a lesser degree, because it's a straight port from the changes that were made in the D7 backport of Edit to support Backbone 1.x.

This is built on top of #2159965: Fix two memory leaks + subtle bugs in Edit's JS (discovered by working on the Backbone upgrade in the D7 backport), so marking postponed.

Note that this does NOT bring the "production" version of Backbone/Underscore, because they (appropriately) reference a source map file (supported by Google Chrome: makes it easy to debug minified JS) and ideally we'd include those in Drupal core. That's scope creep here, so let's just focus on making the upgrade to Backbone 1.1 here and then worry about the minified version later.

jessebeach’s picture

diff --git a/core/modules/ckeditor/js/ckeditor.admin.js b/core/modules/ckeditor/js/ckeditor.admin.js
index ccf2f71..49ca229 100644
--- a/core/modules/ckeditor/js/ckeditor.admin.js
+++ b/core/modules/ckeditor/js/ckeditor.admin.js
@@ -120,8 +120,8 @@ Drupal.ckeditor = {
       this.getCKEditorFeatures(this.model.get('hiddenEditorConfig'), this.disableFeaturesDisallowedByFilters.bind(this));
 
       // Push the active editor configuration to the textarea.
-      this.model.on('change:activeEditorConfig', this.model.sync, this.model);
-      this.model.on('change:isDirty', this.parseEditorDOM, this);
+      this.model.listenTo(this.model, 'change:activeEditorConfig', this.model.sync, this.model);
+      this.listenTo(this.model, 'change:isDirty', this.parseEditorDOM, this);
     },

I think that, in the code above

+      this.model.listenTo(this.model, 'change:activeEditorConfig', this.model.sync, this.model);

should be

+      this.listenTo(this.model, 'change:activeEditorConfig', this.model.sync, this.model);
jessebeach’s picture

Actually, strike that comment in #13. On further inspection, the intent is to bind the listenTo to the model, not the view. The event handler for this line refers to a method on the model, not the view.

BUT, there might be some extraneous code here. We don't need to bind the event handler to the object through an argument, which is what the fourth argument in these two lines would suggest:

// Push the active editor configuration to the textarea.
this.model.listenTo(this.model, 'change:activeEditorConfig', this.model.sync, this.model);
this.listenTo(this.model, 'change:isDirty', this.parseEditorDOM, this);

With listenTo, the handler is bound to the object that invokes listenTo.

[listenTo](http://backbonejs.org/#Events-listenTo): Tell an object to listen to a particular event on an other object...The callback will always be called with object as context.

Honestly, that quote from the docs is....damn muddled. Which object now? The object invoking listenTo or the one raising the event? Let's check the code in Backbone...

var listenMethods = {listenTo: 'on', listenToOnce: 'once'};

// Inversion-of-control versions of `on` and `once`. Tell *this* object to
// listen to an event in another object ... keeping track of what it's
// listening to.
_.each(listenMethods, function(implementation, method) {
  Events[method] = function(obj, name, callback) {
    var listeningTo = this._listeningTo || (this._listeningTo = {});
    var id = obj._listenId || (obj._listenId = _.uniqueId('l'));
    listeningTo[id] = obj;
    if (!callback && typeof name === 'object') callback = this;
    obj[implementation](name, callback, this);
    return this;
  };
});

So, the pertinent line is

obj[implementation](name, callback, this);

where implementation is on and this is the object that invoked listenTo.

The fourth argument passed to listenTo in the code changes above is never referenced in Backbone and just silently passes into the æther.

Wim Leers’s picture

FileSize
150.79 KB
919 bytes

#14: So stupid that I did that conversion incompletely. I went through the exact same sequence of "wtf are these docs saying now?" to just looking at the code and coming to the same conclusion. (I even commented about that on the somewhat-authoritative StackOverflow post about this: http://stackoverflow.com/questions/16823746/backbone-js-listento-vs-on#c....)
Here's a pull request to clarify that: https://github.com/jashkenas/backbone/pull/2935.

webchick’s picture

Status: Postponed » Needs review

That got committed, so back to "needs review."

jessebeach’s picture

There's one more instance in EntityModel.js. I searched with the following regex:

listenTo\(.*,.*,.*,.*\)
Wim Leers’s picture

FileSize
150.79 KB
726 bytes

Good catch!

The last submitted patch, 18: backbone_upgrade-1800022-18.patch, failed testing.

Wim Leers’s picture

Argh, I managed to include the patch for #2159965: Fix two memory leaks + subtle bugs in Edit's JS (discovered by working on the Backbone upgrade in the D7 backport) (that landed already) in this patch, hence it of course does not apply. Stupid stupid stupid.

Wim Leers’s picture

Hopefully less braindead.

Status: Needs review » Needs work

The last submitted patch, 21: backbone_upgrade-1800022-21.patch, failed testing.

herom’s picture

Status: Needs work » Needs review
FileSize
139.3 KB
1.16 KB

core/modules/edit/js/models/BaseModel.js was lost between #18 and #21. adding back.

jessebeach’s picture

Status: Needs review » Reviewed & tested by the community

Alright, this looks good! If this is going to create any issues (I don't think it will), I'd rather know them sooner rather than later.

webchick’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs manual testing

Let's get some manual testing on this first. Navbar, Edit, and contextual links should all be clicked around in a few browsers to make sure they still work.

Wim Leers’s picture

#25: agreed. I already did that, but somebody else should do that too, to make sure I didn't miss anything.

herom’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

I tested this patch, and compared with a clean install, on firefox and chrome.
everything I checked was identical (including those mentioned in #25).

catch’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for testing. Committed/pushed to 8.x, thanks!

nod_’s picture

Same here, looks like everything working fine. RTBC+1

Too late, anyway :p

Wim Leers’s picture

Issue tags: -sprint

That was fast — thanks all!

Status: Fixed » Closed (fixed)

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