WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

#23262 closed task (blessed) (fixed)

Update Backbone.js to 0.9.10

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

Download all attachments as: .zip

Change History (38)

@aaronholbrook3 years ago

23262.patch

@aaronholbrook3 years ago

@aaronholbrook3 years ago

comment:1 @aaronholbrook3 years ago

  • Keywords has-patch added; needs-patch removed

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

comment:2 @aaronholbrook3 years ago

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

@aaronholbrook3 years ago

comment:3 @aaronholbrook3 years ago

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

Thanks Evan :)

@aaronholbrook3 years ago

comment:4 @scribu3 years ago

  • Keywords commit added

comment:5 @nacin3 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.

comment:6 @markoheijnen3 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.

comment:7 @scribu3 years ago

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

comment:8 @scribu3 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.

comment:9 @markoheijnen3 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

@scribu3 years ago

comment:10 @scribu3 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.

comment:11 @markjaquith3 years ago

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

comment:12 @markjaquith3 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

comment:13 @markjaquith3 years ago

In 23383:

Update Underscore to version 1.4.4. see #23262

comment:14 @scribu3 years ago

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

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

comment:15 @Ipstenu3 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.

comment:16 @davouch3 years ago

#23386 was marked as a duplicate.

comment:17 @scribu3 years ago

  • Severity changed from normal to blocker

comment:18 @davouch3 years ago

  • Cc david@… added

comment:19 @azaozz3 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.

comment:21 @scribu3 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.

comment:22 @azaozz3 years ago

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

comment:23 @redsweater3 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()?

comment:24 @Ipstenu3 years ago

Can we revert this to unbreak trunk?

comment:25 @markjaquith3 years ago

In 23390:

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

comment:26 @scribu3 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.

comment:27 @kadamwhite2 years ago

  • Cc kadamwhite added

comment:28 @gcorne2 years ago

  • Cc gregory@… added

comment:29 @gcorne2 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.

Version 0, edited 2 years ago by gcorne (next)

@gcorne2 years ago

comment:30 @koopersmith2 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.