Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#34350 closed task (blessed) (fixed)

Update Backbone to 1.2.3 and Underscore to 1.8.3

Reported by: caseypatrickdriscoll's profile caseypatrickdriscoll Owned by: adamsilverstein's profile adamsilverstein
Milestone: 4.5 Priority: normal
Severity: normal Version: 4.4
Component: External Libraries Keywords: needs-testing has-patch
Focuses: javascript Cc:

Description

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

Attachments (14)

34350.diff (113.7 KB) - added by adamsilverstein 8 years ago.
34350.2.diff (215.5 KB) - added by adamsilverstein 8 years ago.
34350.3.diff (215.9 KB) - added by adamsilverstein 8 years ago.
34350.4.diff (215.5 KB) - added by ericlewis 8 years ago.
34350.mp4 (521.8 KB) - added by ericlewis 8 years ago.
34350.add-audio-source.mp4 (4.3 MB) - added by ocean90 8 years ago.
34350.5.diff (614 bytes) - added by adamsilverstein 8 years ago.
34350.6.diff (2.1 KB) - added by adamsilverstein 8 years ago.
34350.7.diff (745.1 KB) - added by adamsilverstein 8 years ago.
34350.8.diff (745.5 KB) - added by adamsilverstein 8 years ago.
34350.9.diff (654 bytes) - added by adamsilverstein 8 years ago.
34350.10.patch (1.7 KB) - added by imath 8 years ago.
34350.10b.patch (1.7 KB) - added by imath 8 years ago.
34350.10.diff (2.1 KB) - added by adamsilverstein 8 years ago.

Change History (80)

#1 @ocean90
8 years ago

  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to Future Release

Hello caseypatrickdriscoll, welcome to Trac!

Not sure if it will cause issues.

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

#2 @adamsilverstein
8 years ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

#3 @adamsilverstein
8 years ago

  • Keywords has-patch added

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

#4 @adamsilverstein
8 years ago

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)

#5 @adamsilverstein
8 years ago

  • Keywords dev-feedback added

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.

#6 @johnbillion
8 years ago

  • Keywords 4.5-early added; dev-feedback removed

#7 follow-ups: @ericlewis
8 years ago

  • Keywords needs-testing has-patch 4.5-early removed

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.

#8 in reply to: ↑ 7 @adamsilverstein
8 years ago

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.

#9 in reply to: ↑ 7 @adamsilverstein
8 years ago

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.

#10 @adamsilverstein
8 years ago

@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!

#11 follow-up: @ericlewis
8 years ago

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

#12 in reply to: ↑ 11 @adamsilverstein
8 years ago

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.

#13 @adamsilverstein
8 years ago

  • Milestone changed from Future Release to 4.5
  • Summary changed from Update Backbone to 1.2.3 to Update Backbone to 1.2.3 and Underscore to 1.8.3

#14 @adamsilverstein
8 years ago

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:

http://cl.ly/2P103C2s3X38/Revisions__test__WordPress_2016-01-14_16-19-50.jpg

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:

http://cl.ly/1k0a4647252e/Revisions__test__WordPress_2016-01-14_16-18-16.jpg

After upgrading to Underscore.js 1.8.3 I get:

http://cl.ly/241L3S1p2t1z/Revisions__test__WordPress_2016-01-14_16-18-39.jpg

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.

#15 @adamsilverstein
8 years ago

  • Keywords needs-testing has-patch added

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

#16 @ericlewis
8 years ago

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.

#17 @ericlewis
8 years ago

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.

#18 @ericlewis
8 years ago

#31514 was marked as a duplicate.

#19 @ericlewis
8 years ago

From @ocean90 on #31514:

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

@ericlewis
8 years ago

#20 follow-up: @ericlewis
8 years ago

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.

#21 follow-up: @ocean90
8 years ago

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.

#22 @ericlewis
8 years ago

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

#23 in reply to: ↑ 21 ; follow-up: @adamsilverstein
8 years ago

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?

#24 in reply to: ↑ 20 @adamsilverstein
8 years ago

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
8 years ago

#25 follow-up: @ericlewis
8 years ago

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

#26 @ericlewis
8 years ago

  • Keywords needs-patch added; has-patch removed

#27 in reply to: ↑ 23 ; follow-up: @ocean90
8 years ago

Replying to adamsilverstein:

@ocean90: can you describe steps to reproduce?

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

#28 in reply to: ↑ 27 @adamsilverstein
8 years ago

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

Perfect, thanks. I will dig!

#29 in reply to: ↑ 25 ; follow-up: @adamsilverstein
8 years ago

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.

#30 in reply to: ↑ 29 @ericlewis
8 years ago

Replying to adamsilverstein:

@ericlewis assume you mean trunk + patch applied?

Yep!

#31 follow-up: @adamsilverstein
8 years ago

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

Last edited 8 years ago by adamsilverstein (previous) (diff)

#32 follow-up: @adamsilverstein
8 years ago

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

#33 in reply to: ↑ 31 ; follow-up: @ericlewis
8 years ago

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

#34 in reply to: ↑ 32 ; follow-ups: @ocean90
8 years ago

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.

#35 in reply to: ↑ 34 ; follow-ups: @ocean90
8 years ago

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.

#36 in reply to: ↑ 35 @adamsilverstein
8 years ago

Replying to ocean90:

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

Oh, fun! I'll take a look.

#37 in reply to: ↑ 34 @adamsilverstein
8 years ago

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 :)

#38 in reply to: ↑ 33 @adamsilverstein
8 years ago

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.

#39 @adamsilverstein
8 years ago

  • Keywords has-patch added; needs-patch removed

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.

#40 in reply to: ↑ 35 @adamsilverstein
8 years ago

Replying to ocean90:

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

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

#41 @adamsilverstein
8 years ago

@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!

#42 follow-up: @ocean90
8 years ago

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

#43 in reply to: ↑ 42 @adamsilverstein
8 years ago

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.

#44 @adamsilverstein
8 years ago

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

#45 @jorbin
8 years ago

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.

#46 @adamsilverstein
8 years ago

  • Keywords commit added

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.

#47 @ocean90
8 years ago

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

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.

#48 @ocean90
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#49 @adamsilverstein
8 years ago

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

#50 follow-up: @imath
8 years ago

@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
8 years ago

#51 @imath
8 years ago

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.

#52 in reply to: ↑ 50 ; follow-up: @adamsilverstein
8 years ago

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

#53 in reply to: ↑ 52 @imath
8 years ago

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.

@imath
8 years ago

#54 @ocean90
8 years ago

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.

#55 @adamsilverstein
8 years ago

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.

#56 @ocean90
8 years ago

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.

#57 @ocean90
8 years ago

  • Keywords commit removed

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

#58 follow-up: @imath
8 years ago

@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

#59 @ocean90
8 years ago

  • Type changed from enhancement to task (blessed)

#60 @westonruter
8 years ago

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

#61 follow-up: @westonruter
8 years ago

In 36675:

Customize: Fix nav menu item search after Backbone update.

Fixes regression introduced in [36546].

See #34350.

#62 in reply to: ↑ 61 @adamsilverstein
8 years ago

Fixes regression introduced in [36546].

@westonruter thanks for fixing that!

#63 @ocean90
8 years ago

In 36681:

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

Props imath.
See #34350.

#64 in reply to: ↑ 58 @ocean90
8 years ago

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.

#65 @imath
8 years ago

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

#66 @ocean90
8 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.