Opened 4 months ago

Closed 3 months ago

#23262 closed task (blessed) (fixed)

Update Backbone.js to 0.9.10

Reported by: markjaquith Owned by: nacin
Priority: normal Milestone: 3.6
Component: External Libraries Version: 3.5
Severity: normal Keywords: needs-patch
Cc: david@…, kadamwhite, gregory@…

Description

Backbone.js 0.9.9 and 0.9.10 have some cool stuff. We should stay up to date.

Attachments (8)

23262.patch (753 bytes) - added by aaronholbrook 4 months ago.
23262.patch
23262.2.patch (753 bytes) - added by aaronholbrook 4 months ago.
backbone-min.js (17.9 KB) - added by aaronholbrook 4 months ago.
23262.3.patch (34.8 KB) - added by aaronholbrook 4 months ago.
23262.4.patch (34.8 KB) - added by aaronholbrook 4 months ago.
underscore-1.4.4.diff (26.5 KB) - added by scribu 4 months ago.
underscore-1.4.4-fixed.diff (26.6 KB) - added by markjaquith 4 months ago.
23262.5.patch (47.5 KB) - added by gcorne 3 months ago.

Download all attachments as: .zip

Change History (38)

23262.patch

  • Keywords has-patch added; needs-patch removed

Apologies for the double upload, first time manually uploading a diff.

Please ignore the backbone-min.js file, after talking with @evan, realized keeping the WP name was the smart thing to do.

Fixed array styling (spaces between parens) at suggestion of Mr. Solomon.

Thanks Evan :)

  • Keywords commit added
  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 23375:

Update Backbone.js to 0.9.10. props aaronholbrook, fixes #23262.

  • Keywords has-patch commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Media doesn't work for me on several trunk sites. Reverting this change does work for me.

Works fine for me in both Firefox and Chrome. Could you describe how exactly it "doesn't work"? Any JS errors?

Nevermind, I was still running the old Backbone version. When I press the "Add Media" button, I get a JS error:

TypeError: e is undefined

from underscore.min.js.

Add media button:

Uncaught TypeError: Cannot read property 'map' of undefined underscore.min.js:5
T.map.T.collect underscore.min.js:5
T.(anonymous function) underscore.min.js:5
media.model.Attachments.Backbone.Collection.extend._changeFilteredProps media-models.js:395
w backbone.min.js:8
g.Events.trigger backbone.min.js:10
f.extend.set backbone.min.js:14
media.model.Attachments.Backbone.Collection.extend.initialize media-models.js:353
media.model.Query.Attachments.extend.initialize media-models.js:661
g.Collection backbone.min.js:18
d backbone.min.js:41
d backbone.min.js:41
media.model.Query.Attachments.extend.get media-models.js:833
media.model.Attachments.Backbone.Collection.extend._requery media-models.js:556
media.model.Attachments.Backbone.Collection.extend._changeQuery media-models.js:383
w backbone.min.js:8
g.Events.trigger backbone.min.js:10
f.extend.set backbone.min.js:13
media.model.Attachments.Backbone.Collection.extend.initialize media-models.js:353
g.Collection backbone.min.js:18
d backbone.min.js:41
media.query media-models.js:642
media.view.MediaFrame.Post.media.view.MediaFrame.Select.extend.createStates media-views.js:1611
media.view.MediaFrame.Select.media.view.MediaFrame.extend.initialize media-views.js:1486
media.view.MediaFrame.Post.media.view.MediaFrame.Select.extend.initialize media-views.js:1595
g.View backbone.min.js:36
media.View.Backbone.View.extend.constructor media-views.js:1190
d backbone.min.js:41
d backbone.min.js:41
d backbone.min.js:41
d backbone.min.js:41
wp.media media-models.js:30
wp.media.editor.add media-editor.js:455
wp.media.editor.open media-editor.js:622
(anonymous function) media-editor.js:641
v.event.dispatch jquery.js:2
o.handle.u jquery.js:2

Featured image

Uncaught TypeError: Cannot call method 'on' of undefined media-views.js:319
media.controller.State.Backbone.Model.extend._updateMenu media-views.js:319
w backbone.min.js:8
g.Events.trigger backbone.min.js:10
f.extend.set backbone.min.js:13
g.Model backbone.min.js:12
media.controller.State.Backbone.Model.extend.constructor media-views.js:229
d backbone.min.js:41
d backbone.min.js:41
wp.media.featuredImage.frame media-editor.js:351
$.on.on.wp.media.view.settings.post.featuredImageId media-editor.js:382
v.event.dispatch jquery.js:2
o.handle.u jquery.js:2

scribu4 months ago

Updating to underscore 1.4.4 makes the featured image link work again. see underscore-1.4.4.diff.

However, Add Media still doesn't work and no JS error now, which sucks.

Your patch had malformed JS, scribu. Still have issues, but at least underscore parses.

This is the Backbone commit that broke it. The issue appears to be unrelated to Underscore.

https://github.com/documentcloud/backbone/commit/71b6404d73f506627f0cc516177e39cc8c9947d7

In 23383:

Update Underscore to version 1.4.4. see #23262

Getting a different error after [23383], in Firefox:

TypeError: this.frame is undefined
this.frame.on( 'menu:render:' + menu, this._renderMenu, this );

Still broke on Chrome too. I'm about to fly out the door, but I svn'd up and it's still not opening the modal.

#23386 was marked as a duplicate.

  • Severity changed from normal to blocker
  • Cc david@… added

There are quite a few changes in Backbone 0.9.10:

View#make has been removed. You'll need to use $ directly to construct DOM elements now.

We use that in few places.

The Model#change method has been removed, as delayed attribute changes as no longer available.

And that.

Also it seems change event on collections now fires on creating the collection, before we can add this.frame throwing the above error.

I think we should revert [23383] and [23375], so that trunk doesn't stay broken until we come up with a properly tested patch.

Think so too. Had a better look and even after patching the obvious places, there are still errors.

I looked into this briefly. The firing of the change event as alluded to by azaozz above does indeed cause the this._updateMenu callback to be reached earlier than it was previously.

I don't know enough about the code, or about Backbone to make very informed choices about reorganizing this, but I did notice for example if I move the line:

this.on( 'change:menu', this._updateMenu, this );

Out of the media controller's constructor, and into _ready():

_ready: function() {
	this._updateMenu();
	this.on( 'change:menu', this._updateMenu, this );
},

Then at least the UI is allowed to complete instantiation without exceptions, and the media window appears. Of course, as predicted, there are further issues within once you try to navigate around the media window.

Seems like more than one nuanced bad behavior may boil down to assumptions in different controllers that change events will come after the add events. In this particular case, doesn't it make sense to at least push off the registration of the this._updateMenu callback until after the menu has been initialized in _ready()?

Can we revert this to unbreak trunk?

In 23390:

Revert [23375] until we can get Media fixed to work with newer versions of Backbone. see #23262

  • Keywords needs-patch added
  • Severity changed from blocker to normal

It seems the underscore.js upgrade didn't have any adverse effects, so we don't need to revert that.

  • Cc kadamwhite added
  • Cc gregory@… added

I spent some time looking at this today. In addition to the Backbone changes that @azaozz mentioned above, there are also some changes to Backbone.sync that impact the overridden .sync() methods and .getByCid() has been removed in favor of just using .get()

I put together a patch that seems to be working, although I haven't tested it extensively.

Version 0, edited 3 months ago by gcorne (next)

gcorne3 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 23589:

Update Backbone to 0.9.10. Update media to handle breaking changes.

  • When overriding Backbone.sync(), ensure the sync and error events fire consistently.
  • Model#make() has been removed. Use $ instead, and be sure to grab the DOM node where necessary (using [0]).
  • Collection#get() now accepts cids. Collection#getByCid() has been removed.
  • When overriding the State constructor, bind change callbacks after the default Model constructor is called, because the Model constructor no longer passes the silent flag when calling set() for the default attributes.
  • In 'change' events, options.changes was removed. It can now be accessed through model.changed. Check if any attributes have changed by calling model.hasChanged(). Also, don't mess with model.changed; it persists beyond the scope of a single event.
  • options.index is no longer be set in the add event callback. Use collection.indexOf(model) can be used to retrieve the index of a model instead.

props gcorne. fixes #23262.

Note: See TracTickets for help on using tickets.