Update Backbone to 1.2.3 and Underscore to 1.8.3

[caseypatrickdriscoll]

Current version is at 1.1.2 from Feb 2014 https://core.trac.wordpress.org/changeset/27233

Will probably need to update Underscores (unless it is baked in)

Not sure if it will cause issues.

​http://backbonejs.org/#changelog

[ocean90]

Hello caseypatrickdriscoll, welcome to Trac!

Not sure if it will cause issues.

Would you like to test if it will cause any issues?

[adamsilverstein]

34350.diff​ updates backbone.js and backbone.min.js to latest version.

[adamsilverstein]

Initial testing, some console errors after upgrading, we may need to upgrade underscore as well.

• In customizer and media library grid view, I see

underscore.js Uncaught TypeError: Cannot use 'in' operator to search for 'model' in undefined(anonymous function)

[adamsilverstein]

34350.2.diff​:

• Update Underscore.js to latest version 1.8.3

Corrects console errors; quickly tested media grid, upload, theme browser, revisions & customizer. Deserves more extensive testing.

Lets get this in at the beginning of the 4.5 release cycle.

[ericlewis]

Using Backbone 1.2.3 and Underscore 1.8.3 doesn't produce console warnings but breaks at least Media and Revisions.

• Clicking "Add New" in Media Library does nothing
• Can't upload media in modal
• Revisions browser slider doesn't load

Looking through ​Backbone 1.1.2-1.2.3 and ​Underscore 1.6.0-1.8.3 changelogs. Nothing jumps off the page but somewhere there's at least one breaking change.

[adamsilverstein]

Looking through ​Backbone 1.1.2-1.2.3 and ​Underscore 1.6.0-1.8.3 changelogs. Nothing jumps off the page but somewhere there's at least one breaking change.

Ugh! Thanks for testing; it would be great to get libraries upgraded - I will spend some time trying to determine what is going wrong: I can start with the revisions slider I know well, maybe that will reveal something helpful for fixing media.

[adamsilverstein]

Replying to ericlewis:

Using Backbone 1.2.3 and Underscore 1.8.3 doesn't produce console warnings but breaks at least Media and Revisions.

I wasn’t able to reproduce the media issues, are you sure you don't have anything else running that could interfere with media uploads? Seemed to work fine here. Confirmed revisions slider issue, may be a jQuery UI thing, thats what we are using for the slider.

[adamsilverstein]

@ericlewis
In 34350.3.diff​ my only additional change is to extract values before flattening the array of subviews returned in wp.Backbone.Subviews.add - this fixes the slider not rendering issue: the ready events where never getting fired in the subviews because all() was returning an empty array.

All the subviews are being added using a blank selector and flatten was returning an empty array. I'm going to test a few more things: using the existing versions of bb/us with this change, and tracing what happens in the older versions: whats changed. I'm guessing Underscore has changed how its flatten works although nothing popped out at me when reviewing the change logs.

Can you test this patch to see if it resolves the issues you were seeing in media? subviews should now get their ready events fired which should help!

[ericlewis]

Can you test this patch to see if it resolves the issues you were seeing in media? subviews should now get their ready events fired which should help!

I can't reproduce the issue with your fix :)

I'm going to test a few more things: using the existing versions of bb/us with this change, and tracing what happens in the older versions: whats changed. I'm guessing Underscore has changed how its flatten works although nothing popped out at me when reviewing the change logs.

This would be most excellent. We'll need to understand the breaking change for both core and for the community

[adamsilverstein]

This would be most excellent. We'll need to understand the breaking change for both core and for the community

Yea, it would satisfy my own curiosity as well. Since i've narrowed it down to that flatten statement, I should be able to figure out whats happening with a little more digging.

[adamsilverstein]

I verified this is due to a change in Underscore's _flatten function.

wp.Backbone.Subviews keeps track of all the views you add to a parent view. the subviews are tracked by their DOM element id or a blank string if they aren't stored against an ID. On the revisions page that looks like this:

I wrote a tiny bit of test code to see what happens when I _.flatten an array that is indexed with a blank string.

With Underscore.js 1.6.0 I get:

After upgrading to Underscore.js 1.8.3 I get:

This breaks the way wp.backbone.Views/Subviews stores views.

I'm going to keep digging to see if this is a is an intentional change in Underscore or a bug.

[adamsilverstein]

@ericlewis

flatten functionality is pretty different in the latest Underscore;

however: our use is incorrect as _.flatten is designed to work on an array, not an object. We were passing it an object and were lucky it even worked. We need to address this on our end.

Grabbing all the values here looks like its the right thing to do, in my testing its doing the right thing - returning all of the registered subviews. I'd like to see some more testing with all of our wp.backbone powered components to make sure nothing else crops up.

Once we are satisfied that nothing major is broken, lets merge to get some wider testing.

[ericlewis]

Great work here @adamsilverstein!

This changed in Underscore.js 1.7. _.flatten() working with objects was an unintended side-affect of the implementation, see ​underscore#1904.

[ericlewis]

In 36305:

In wp.Backbone.Subviews, extract subviews with proper Underscore.js functions.

Subviews are stored internally on the Subview manager as an object. The object
is composed of key-value pairs where the key is a jQuery selector for a view,
and the value is an array of views that matching the selector.

To extract subviews, _.flatten() was used to collate the nested arrays of
views into a single view. However, _.flatten() is not intended to be used
for objects, and this unintended functionality breaks in newer versions of
Underscore.js.

Instead, we'll use _.values() to extract the arrays of views first,
and then flatten the array of arrays.

Props adamsilverstein.
See #34350.

[ericlewis]

#31514 was marked as a duplicate.

[ericlewis]

From @ocean90 on #31514:

[Underscore.js 1.8.2] breaks loading the Customizer and inserting a gallery via the media modal.

[ericlewis]

Updated the diff in attachment:34350.4.diff​ after r36305.

Replying to adamsilverstein:

Can you test this patch to see if it resolves the issues you were seeing in media? subviews should now get their ready events fired which should help!

This seems to come and go, but I can still reproduce this. Clicking Add New in the Media Library does nothing. I see that the ​listenTo is invoked which should set up event binding, and the ​clickhandler gets fired which triggers the event, but something's getting mucked up in between.

[ocean90]

With 34350.4.diff​ applied the "Add Media" button is working for me, uploads too. @ericlewis where are you seeing a "Add New" button? Or is it "Add Media"?

I noticed another issue: The replace/remove button/link (​https://cloudup.com/cQMabKwimsJ) for audio sources doesn't work always.

[ericlewis]

@ericlewis where are you seeing a "Add New" button? Or is it "Add Media"?

In the Media Library, i.e. the Media Grid. Is there preferred nomenclature for this page? Never knew what to call it.

[adamsilverstein]

I noticed another issue: The replace/remove button/link (​https://cloudup.com/cQMabKwimsJ) for audio sources doesn't work always.

@ocean90: can you describe steps to reproduce?

[adamsilverstein]

This seems to come and go, but I can still reproduce this. Clicking Add New in the Media Library does nothing.

Gotta love those intermittent issues! Any special steps to reproduce, or just keep visiting the media library and trying to upload? Any console errors?

[ericlewis]

Added a screencast in attachment:34350.mp4​. Nothing in the console, on trunk.

[ocean90]

Replying to adamsilverstein:

@ocean90: can you describe steps to reproduce?

How about a screencast? 34350.add-audio-source.mp4​

[adamsilverstein]

How about a screencast? 34350.add-audio-source.mp4​

Perfect, thanks. I will dig!

[adamsilverstein]

Added a screencast in attachment:34350.mp4​. Nothing in the console, on trunk.

@ericlewis assume you mean trunk + patch applied? Thanks for the screencast, I will dig in and try to track down the breakage.

[ericlewis]

Replying to adamsilverstein:

@ericlewis assume you mean trunk + patch applied?

Yep!

[adamsilverstein]

@ericlewis tracked that issue down to the binding of the click handler. the listenTo must have been getting eaten by a view refresh. Not sure why upgrading us/bb breaks this: this patch - 34350.5.diff​ - fixes it by switching to on for binding

Makes me think we need to do more testing; would also love to understand why this broke.

[adamsilverstein]

@ocean90 can you try patch 34350.6.diff​ to see if that resolves your issue (I'm confused if i patch media-audiovideo.js or media-details.js so i did both (how are these built?)

As of Backbone 1.2.0 "You can no longer modify the events hash or your view's el property in initialize."

We should hunt for other instances where we might be doing this.

[ericlewis]

I ran a git bisect on Backbone and tracked the breaking change down to ​this commit.

[ocean90]

Replying to adamsilverstein:

@ocean90 can you try patch 34350.6.diff​ to see if that resolves your issue (I'm confused if i patch media-audiovideo.js or media-details.js so i did both (how are these built?)

Run grunt watch and change the files in /src/wp-includes/js/media. Before creating the patch run grunt precommit once.

34350.6.diff​ seems to work, not sure if the function() wrapper is really needed here. Using _.defaults() like in trunk/src/wp-includes/js/media/views/image-details.js?marks=18-26#L15 works too.

[ocean90]

Speaking of grunt precommit, it fails with 34350.4.diff​ applied:

Running "qunit:files" (qunit) task
Testing tests/qunit/compiled.html ............................................................................................................................................................F.
>> Customize Widgets - widget contents should embed (with widget-added event) when section and control expand
>> Message: Died on test #3     at file:///Users/Dominik/Development/WordPress/vagrant/www/wp-develop/svn/tests/qunit/vendor/qunit.js:263
>>     at file:///Users/Dominik/Development/WordPress/vagrant/www/wp-develop/svn/tests/qunit/vendor/sinon-qunit.js:61
>>     at file:///Users/Dominik/Development/WordPress/vagrant/www/wp-develop/svn/tests/qunit/wp-admin/js/customize-widgets.js:54
>>     at file:///Users/Dominik/Development/WordPress/vagrant/www/wp-develop/svn/build/wp-includes/js/jquery/jquery.js:3
>>     at file:///Users/Dominik/Development/WordPress/vagrant/www/wp-develop/svn/build/wp-includes/js/jquery/jquery.js:3: Can't find variable: sidebars
>> Actual: null
>> Expected: undefined
>> ReferenceError: Can't find variable: sidebars

Testing tests/qunit/index.html ............................................................................................................................................................F.
>> Customize Widgets - widget contents should embed (with widget-added event) when section and control expand
>> Message: Died on test #3     at file:///Users/Dominik/Development/WordPress/vagrant/www/wp-develop/svn/tests/qunit/vendor/qunit.js:263
>>     at file:///Users/Dominik/Development/WordPress/vagrant/www/wp-develop/svn/tests/qunit/vendor/sinon-qunit.js:61
>>     at file:///Users/Dominik/Development/WordPress/vagrant/www/wp-develop/svn/tests/qunit/wp-admin/js/customize-widgets.js:54
>>     at file:///Users/Dominik/Development/WordPress/vagrant/www/wp-develop/svn/src/wp-includes/js/jquery/jquery.js:3
>>     at file:///Users/Dominik/Development/WordPress/vagrant/www/wp-develop/svn/src/wp-includes/js/jquery/jquery.js:3: Can't find variable: sidebars
>> Actual: null
>> Expected: undefined
>> ReferenceError: Can't find variable: sidebars

Warning: 2/736 assertions failed (3599ms) Use --force to continue.

Aborted due to warnings.

[adamsilverstein]

Replying to ocean90:

Speaking of grunt precommit, it fails with 34350.4.diff​ applied:

Oh, fun! I'll take a look.

[adamsilverstein]

34350.6.diff​ seems to work, not sure if the function() wrapper is really needed here. Using _.defaults() like in trunk/src/wp-includes/js/media/views/image-details.js?marks=18-26#L15 works too.

I found the function to be required to extend the prototype events. In my testing without this, other buttons on the details pane that fail to work. events takes either an object or a function, maybe the function gets evaluated later? in any case, if you try switching to not use the function I think you will find the events attached to the prototype don't fire. If thats not the case, agree that would be cleaner!

Also, thanks for the build info :)

[adamsilverstein]

Replying to ericlewis:

I ran a git bisect on Backbone and tracked the breaking change down to ​this commit.

Nice work tracking that down! Yea git bisect!

Wow, thats a pretty big change to Backbone events, no wonder things broke. We should audit our use of events to make sure nothing else broke.

[adamsilverstein]

In 34350.7.diff​ :

• Rollup previous patches
• Corrects the use of _.template in customize-widgets.js; since Underscore 1.7 Underscore templates no longer accept an initial data object. _.template always returns a function now. See the ​diff for this file.

[adamsilverstein]

Replying to ocean90:

Speaking of grunt precommit, it fails with 34350.4.diff​ applied:

Fixed in 34350.7.diff​; can you please re-test?

[adamsilverstein]

@ocean90 & @ericlewis do either of you see any remaining issue?

Can we commit this before the merge window closes? I would love to get it into core so it gets some wider testing. I really believe we need to fix any incompatibilities in our code at this point rather than holding back bringing these libraries current; hopefully any issues that come up can be quickly resolved... if not, we can always revert!

[ocean90]

34350.5.diff​ isn't included in 34350.7.diff​, any reason for that?

[adamsilverstein]

Replying to ocean90:

34350.5.diff​ isn't included in 34350.7.diff​, any reason for that?

no, should have been; thats a mistake, resubmitting.

[adamsilverstein]

In 34350.8.diff​: include 34350.5.diff in rollup patch.

[jorbin]

Quick note to whoever commits this, it is going to require a make/core post so that plugin authors know to test backbone and underscore.

[adamsilverstein]

Here is a summary of this upgrade which could be useful for a commit message and expanded to a make post.

---

Update Backbone and Underscore to the latest versions. Backbone, from 1.1.2 to 1.2.3. Underscore, from 1.6.0 to 1.8.3.

The new versions of Backbone and Underscore offer numerous small bug fixes and some optimizations and other improvements. Check the ​Backbone changelog and ​Underscore changelog for the full details.

The new versions include some significant changes that may break existing code. Plugins or themes that rely on the bundled Backbone and/or Underscore libraries should carefully check funcitonlity with the latest versions and run any available run unit tests to ensure compatibility.

Some changes of note that were addressed in core as part of this upgrade:

• _.flatten no longer works with objects since Underscore.js 1.7. _.flatten() working with objects was an unintended side-affect of the implementation, see ​underscore#1904. Check any _flatten usage and only flatten arrays.
• As of Backbone 1.2.0, you can no longer modify the events hash or your view's el property in initialize, so don't try to modify them there.
• Since Underscore 1.7, Underscore templates no longer accept an initial data object. _.template always returns a function now so make sure you use it that way.

[ocean90]

In 36546:

Update Backbone and Underscore to the latest versions.

Backbone, from 1.1.2 to 1.2.3. Underscore, from 1.6.0 to 1.8.3.

The new versions of Backbone and Underscore offer numerous small bug fixes and some optimizations and other improvements. Check the ​Backbone changelog and ​Underscore changelog for the full details.

The new versions include some significant changes that may break existing code. Plugins or themes that rely on the bundled Backbone and/or Underscore libraries should carefully check functionality with the latest versions and run any available unit tests to ensure compatibility.

Some changes of note that were addressed in core as part of this upgrade:

• _.flatten no longer works with objects since Underscore.js 1.7. _.flatten() working with objects was an unintended side-affect of the implementation, see ​underscore#1904. Check any _flatten usage and only flatten arrays.
• As of Backbone 1.2.0, you can no longer modify the events hash or your view's el property in initialize, so don't try to modify them there.
• Since Underscore 1.7, Underscore templates no longer accept an initial data object. _.template always returns a function now so make sure you use it that way.

Props adamsilverstein.
Fixes #34350.

[ocean90]

See #35853.

---

Another issue: The search at wp-admin/themes.php doesn't work anymore. It returns no results and if you clear the search input you get a "Uncaught RangeError: Maximum call stack size exceeded" error.

[adamsilverstein]

@ocean90 -- This fixes the theme searching issue:

34350.9.diff​ :

• Use an array when passing items to union.
• Since Underscore 1.7.0 "For consistency, _.union and _.difference now only work with arrays and not variadic args."
• Fixes an issue that prevented theme searching from working since r36546.

[imath]

@adamsilverstein i've found another issue with the Media Library Bulk button. It was no longer showing the 'Delete Selected' & 'Cancel Selection' buttons.
34350.10.patch is bringing back the buttons and it's possible to bulk delete, but the 'Delete Selected' doesn't come back to its initial state when the selection is emptied. I hope this "half way" patch will help you fix this completely.

[imath]

Delete Selected' doesn't come back to its initial state when the selection is emptied. I hope this "half way" patch will help you fix this completely

Well actually, i've tested with previous Backbone/underscore versions and the 'Delete Selected' button wasn't coming back to its original state when the selection was emptied, so it's not a regression, it might be something to consider as an enhancement.

[adamsilverstein]

Replying to imath:

34350.10.patch is bringing back the buttons and it's possible to bulk delete, but the 'Delete Selected' doesn't come back to its initial state when the selection is emptied. I hope this "half way" patch will help you fix this completely.

Great. Thanks for catching these.

In your patch, I don't think you need the third 'context' variable (this), unless the call you are replacing has the _.bind( function, this ) format the function doesn't need this bound and doing so adds overhead and could possibly lead to unexpected behavior (if the function expected this to have a different context).

[imath]

Replying to adamsilverstein:

In your patch, I don't think you need the third 'context' variable (this)

Well, i've tried, the remaining ones in .10b.patch seems to be necessary else the bulk feature doesn't work as expected.

[ocean90]

In 36575:

Media Library: After [36546] restore the "Add new" functionality.

Rework handling of the 'toggle:upload:attachment' event using .on vs .listenTo for better compatibility with the current version of Backbone.

Props adamsilverstein.
See #34350.
Fixes #35853.

[adamsilverstein]

34350.10.diff​

• Fixes search results on themes
• Correct several incorrect uses of _.union (also searched codebase for other uses, there were none); added a note to the blog post as well. (Since Underscore 1.7.0 "For consistency, _.union and _.difference now only work with arrays and not variadic args.")
• Use a namespaced event themes:update vs update. Backbone 1.2 Added a built in update event that triggers after any amount of models are added or removed from a collection. This additional unexpected event was breaking the search results display on themes.php, switching to a namespaced event corrects the issue.

[ocean90]

In 36580:

Themes: After [36546] restore theme search functionality.

• Correct several incorrect uses of _.union. Since Underscore 1.7.0 _.union supports only arrays and not variadic args.
• Use a namespaced event themes:update. Backbone 1.2 added a built in update event that triggers after any amount of models are added or removed from a collection.

Props adamsilverstein.
See #34350.

[ocean90]

@imath Which patch should be used to fix the issue mentioned in comment:50?

[imath]

@ocean90 both are fixing it, the second one is less using the this context variable to only keep it when necessary. I've just retested it and i confirm 34350.10b.patch is fixing the issue

[westonruter]

@adamsilverstein I just found that [36546] broke the nav menu item search functionality in the Customizer.

[westonruter]

In 36675:

Customize: Fix nav menu item search after Backbone update.

Fixes regression introduced in [36546].

See #34350.

[adamsilverstein]

Fixes regression introduced in [36546].

@westonruter thanks for fixing that!

[ocean90]

In 36681:

Media: Fix broken delete/trash functionality in the library after [36546].

Props imath.
See #34350.

[ocean90]

Replying to imath:

@ocean90 both are fixing it, the second one is less using the this context variable to only keep it when necessary. I've just retested it and i confirm 34350.10b.patch is fixing the issue

Actually 34350.10.patch​ was the right one, this was always required. You had to enable media trash to see the errors.

[imath]

@ocean90 thanks a lot for the commit, the props & the info :)