WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 2 months ago

Last modified 6 weeks ago

#47478 closed task (blessed) (fixed)

Update Backbone.js to 1.4.0

Reported by: desrosj Owned by: adamsilverstein
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: External Libraries Keywords: early good-first-bug has-patch has-dev-note
Focuses: Cc:
PR Number:

Description

Backbone.js 1.4.0 was released on February 19, 2019.

Core currently includes version 1.3.3 using the package.json file. However, the version indicated when the script is enqueued incorrectly indicates 1.2.3 is loaded.

A full list of changes since 1.3.3 can be found here: https://github.com/jashkenas/backbone/compare/1.3.3...1.4.0.

Attachments (8)

47478.diff (1.3 KB) - added by priyankkpatel 6 months ago.
Patch for wp-includes/script-loader.php
backbone.js (76.0 KB) - added by priyankkpatel 6 months ago.
backbone.js 1.4.0
backbone.min.js (24.4 KB) - added by priyankkpatel 6 months ago.
backbone.min 1.4.0
package-lock.diff (699 bytes) - added by priyankkpatel 5 months ago.
patch for file package-lock.json
47478-2.diff (2.0 KB) - added by priyankkpatel 5 months ago.
Final patch including changes in package.json, package-lock.json and wp-includes/script-loader.php
47478.3.patch (8.0 KB) - added by pierlo 2 months ago.
47478.2.diff (6.0 KB) - added by adamsilverstein 2 months ago.
47478.3.diff (4.1 KB) - added by adamsilverstein 2 months ago.

Download all attachments as: .zip

Change History (28)

@priyankkpatel
6 months ago

Patch for wp-includes/script-loader.php

#1 @priyankkpatel
6 months ago

I have attached patch for wp-includes/script-loader.php and package.json.
package.json will now download Backbone.js version 1.4.0

@priyankkpatel
6 months ago

backbone.js 1.4.0

@priyankkpatel
6 months ago

backbone.min 1.4.0

This ticket was mentioned in Slack in #core by desrosj. View the logs.


5 months ago

#3 follow-up: @desrosj
5 months ago

  • Keywords has-patch needs-testing needs-refresh added; needs-patch removed
  • Owner set to priyankkpatel
  • Status changed from new to assigned

Assigning to @priyankkpatel to mark good-first-bug claimed.

Thanks for the patch! The package-lock.json file will also need to be updated.

@adamsilverstein are you able to help testing this?

#4 in reply to: ↑ 3 @priyankkpatel
5 months ago

Replying to desrosj:

Assigning to @priyankkpatel to mark good-first-bug claimed.

Thanks for the patch! The package-lock.json file will also need to be updated.

@adamsilverstein are you able to help testing this?

You are welcome @desrosj

@priyankkpatel
5 months ago

patch for file package-lock.json

@priyankkpatel
5 months ago

Final patch including changes in package.json, package-lock.json and wp-includes/script-loader.php

#5 @adamsilverstein
5 months ago

Yes, I'll give this a test soon.

#6 @desrosj
3 months ago

I did some testing with this today. I found a few issues. They all seem to be related to this._listeners and cleaning up memory bindings. Here are scenarios that are showing the issue:

  1. Go to the Media Library and upload an image. When uploading finishes, you'll see the console error.
  2. In the Classic Editor, click Add Media, select an image, click insert. Observe the console error.
  3. In the Classic Editor, click Add Media, click Create Gallery. Observe the console error. The view also does not switch, and subsequent tab changes do not always work. Clicking Create New Gallery does nothing.

These issues also happen when working with the media modals in the block editor.

@pierlo
2 months ago

#7 @pierlo
2 months ago

Took some time out and managed to solve this. The _listeners object was being applied to the states Collection in the StateMachine (aka this.controller), instead of the StateMachine itself. This is because of a change in Backbone v1.4.0 which applies listeners to the object's public on method (in this case the StateMachine maps events and triggers to the states object).

47478.3.patch fixes this by passing the states object instead of the StateMachine. I also refactored StateMachine to remove some dead code.

Last edited 2 months ago by pierlo (previous) (diff)

This ticket was mentioned in Slack in #core by pierlo. View the logs.


2 months ago

#9 @adamsilverstein
2 months ago

  • Keywords needs-dev-note added

Hey @pierlo thanks for the patch and the detailed explanation about the required changes. I tested media and revisions and everything worked as expected after this change.

Since the listener changes in 1.4 might also affect plugin authors, this change deserves a devnote. https://backbonejs.org/#changelog

@desrosj this looks good to me, unless you have any final concerns I can go ahead and commit this.

#10 @desrosj
2 months ago

  • Keywords commit added; needs-testing needs-refresh removed

@adamsilverstein Let's get it in so it has time to sit until beta next week.

Thanks for working on this, @pierlo! Would you be willing to summarize the above in a short paragraph or two for a dev note? Doesn't have to be on this ticket. We can connect in Slack if you'd like.

#11 @pierlo
2 months ago

Sure thing.

#12 @adamsilverstein
2 months ago

  • Owner changed from priyankkpatel to adamsilverstein

Will do.

#13 @adamsilverstein
2 months ago

47478.2.diff limits the package-lock changes to the backbone dependency

#14 @adamsilverstein
2 months ago

47478.3.diff removes unintended changes in class-wp-user-query.php

#15 @adamsilverstein
2 months ago

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

In 46157:

External Libraries: Update Backbone.js to 1.4.0.

Upgrade Backbone to the latest stable version. Fix some issues in Media with listenTo which changed in this version, see https://backbonejs.org/#changelog.

Props desrosj, priyankkpatel, pierlo.
Fixes #47478.

This ticket was mentioned in Slack in #core-js by adamsilverstein. View the logs.


6 weeks ago

#17 @joyously
6 weeks ago

Was the Customizer tested with the new version?

#18 @adamsilverstein
6 weeks ago

Great question @joyously - I did some basic testing in the Customizer: additional testing would be welcome!

In general, I don't think the bigger changes would affect the Customizer.

#19 @desrosj
6 weeks ago

  • Keywords has-dev-note added; needs-dev-note commit removed

#20 @adamsilverstein
6 weeks ago

@joyously Wanted to note that I went back and spent some time testing the customizer with several bundled themes and didn't note any issues. Now that the code is in our released betas, we can expect much wider testing and hopefully we can catch any possible regressions.

Note: See TracTickets for help on using tickets.