Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#23262 closed task (blessed) (fixed)

Update Backbone.js to 0.9.10

Reported by: markjaquith's profile markjaquith Owned by: nacin's profile nacin
Milestone: 3.6 Priority: normal
Severity: normal Version: 3.5
Component: External Libraries Keywords: needs-patch
Focuses: Cc:

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 11 years ago.
23262.patch
23262.2.patch (753 bytes) - added by aaronholbrook 11 years ago.
backbone-min.js (17.9 KB) - added by aaronholbrook 11 years ago.
23262.3.patch (34.8 KB) - added by aaronholbrook 11 years ago.
23262.4.patch (34.8 KB) - added by aaronholbrook 11 years ago.
underscore-1.4.4.diff (26.5 KB) - added by scribu 11 years ago.
underscore-1.4.4-fixed.diff (26.6 KB) - added by markjaquith 11 years ago.
23262.5.patch (47.5 KB) - added by gcorne 11 years ago.

Download all attachments as: .zip

Change History (38)

@aaronholbrook
11 years ago

23262.patch

#1 @aaronholbrook
11 years ago

  • Keywords has-patch added; needs-patch removed

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

#2 @aaronholbrook
11 years ago

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

#3 @aaronholbrook
11 years ago

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

Thanks Evan :)

#4 @scribu
11 years ago

  • Keywords commit added

#5 @nacin
11 years ago

  • 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.

#6 @markoheijnen
11 years ago

  • 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.

#7 @scribu
11 years ago

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

#8 @scribu
11 years ago

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.

#9 @markoheijnen
11 years ago

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

#10 @scribu
11 years 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.

#11 @markjaquith
11 years ago

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

#12 @markjaquith
11 years ago

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

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

#13 @markjaquith
11 years ago

In 23383:

Update Underscore to version 1.4.4. see #23262

#14 @scribu
11 years ago

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

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

#15 @Ipstenu
11 years ago

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.

#16 @davouch
11 years ago

#23386 was marked as a duplicate.

#17 @scribu
11 years ago

  • Severity changed from normal to blocker

#18 @davouch
11 years ago

  • Cc david@… added

#19 @azaozz
11 years ago

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.

#21 @scribu
11 years ago

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

#22 @azaozz
11 years ago

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

#23 @redsweater
11 years ago

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()?

#24 @Ipstenu
11 years ago

Can we revert this to unbreak trunk?

#25 @markjaquith
11 years ago

In 23390:

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

#26 @scribu
11 years ago

  • 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.

#27 @kadamwhite
11 years ago

  • Cc kadamwhite added

#28 @gcorne
11 years ago

  • Cc gregory@… added

#29 @gcorne
11 years ago

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. My work can also be seen in a branch on GitHub https://github.com/gcorne/WordPress/commits/backbone-upgrade for those that prefer a bit more detail.

Last edited 11 years ago by gcorne (previous) (diff)

@gcorne
11 years ago

#30 @koopersmith
11 years 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.