Make WordPress Core

Opened 6 years ago

Closed 3 years ago

Last modified 3 years ago

#45785 closed enhancement (fixed)

Update Underscore to the latest version

Reported by: mukesh27's profile mukesh27 Owned by: davidbaumwald's profile davidbaumwald
Milestone: 5.8 Priority: normal
Severity: normal Version:
Component: External Libraries Keywords: early has-patch
Focuses: Cc:

Description

Let's keep WordPress up-to-date with the latest Underscore version.

WordPress 5.0.2 includes Underscore 1.8.3 as of this report but Underscore 1.9.1 is the latest version.

See Underscore changelog at https://underscorejs.org/#changelog

Attachments (6)

45785.diff (149.8 KB) - added by Hareesh Pillai 5 years ago.
Patch for upgrading underscore to v 1.91.
45785.2.diff (1001 bytes) - added by Hareesh Pillai 5 years ago.
Changes to package.json and package-lock.json file
45785.3.diff (1.9 KB) - added by SergeyBiryukov 4 years ago.
45785.4.diff (2.7 KB) - added by SergeyBiryukov 4 years ago.
45785.5.diff (2.7 KB) - added by SergeyBiryukov 4 years ago.
45785.6.diff (2.2 KB) - added by SergeyBiryukov 4 years ago.

Download all attachments as: .zip

Change History (34)

@Hareesh Pillai
5 years ago

Patch for upgrading underscore to v 1.91.

#1 @Hareesh Pillai
5 years ago

  • Keywords has-patch added

#2 @Hareesh Pillai
5 years ago

  • Keywords early added

#3 @desrosj
5 years ago

  • Keywords needs-refresh added
  • Milestone changed from Awaiting Review to 5.3

Thanks for the patch, @hareesh-pillai!

Underscore is now managed by the package.json file. The version needs to be updated in that file and the appropriate changes to the package-lock.json file also need to be included in the patch.

@Hareesh Pillai
5 years ago

Changes to package.json and package-lock.json file

#4 @desrosj
5 years ago

  • Keywords commit added; needs-refresh removed

Thanks for your work on this, @hareesh-pillai!

I did some testing and discovered a console error. But it turns out this already exists in core. I opened #48021 for that.

#5 @desrosj
5 years ago

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

#6 @desrosj
5 years ago

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

In 46092:

External Libraries: Update Underscore to 1.9.1.

Changes: https://github.com/jashkenas/underscore/compare/1.8.3...1.9.1

Props mukesh27, hareesh-pillai.
Fixes #45785

#7 @SergeyBiryukov
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to address test failures:

>> tinymce.obsolete - HTML elements non-conforming to HTML 5.0
>> Message: TypeError: undefined is not an object (evaluating 'a.replace')
>> Actual: null
>> Expected: undefined
>> file:///home/travis/build/WordPress/wordpress-develop/build/wp-includes/js/underscore.min.js:1

>> tinymce.obsolete - HTML elements non-conforming to HTML 5.0
>> Message: Expected 6 assertions, but 7 were run
>> Actual: null
>> Expected: undefined
>> test@file:///home/travis/build/WordPress/wordpress-develop/tests/qunit/vendor/sinon-qunit.js:61:21

The failures come from tests/qunit/wp-includes/js/tinymce/tinymce-obsolete.js, however renaming that file just leads to similar failures in other files.

It's also worth noting that the failures are inconsistent, sometimes I get a different message:

>> Video Media Widget - video widget control renderPreview                                                                                                  
>> Message: TypeError: undefined is not an object (evaluating 'a.replace')                                                                                  
>> Actual: null                                                                                                                                             
>> Expected: undefined                                                                                                                                      
>> file:///S:/home/wordpress.test/develop.svn.wordpress.org/build/wp-includes/js/underscore.min.js:1   

I was able to track down the issue to this commit cleaning up the _.debounce() function, before that commit the tests pass successfully.

Not sure what's specifically causing the issue here, I guess there's some kind of a race condition between Underscore's _.debounce() and QUnit's internal timeout handling.

#8 @SergeyBiryukov
5 years ago

In 46093:

External Libraries: Revert [46092] pending test failure investigation.

See #45785.

#9 @desrosj
5 years ago

  • Keywords needs-testing added; commit removed
  • Milestone changed from 5.3 to Future Release

I looked at this a bit today. The _.debounce() function invokes _.delay() with 4 arguments, but that function appears to only accept 3 arguments. Removing this from this line in Underscore fixes the problem. It's strange that only one are two tests are failing (and with inconsistent messages), but all of the front end functionality seems to be working as best I can tell.

Going to move this to Future Release for now since this is not a pressing change. If someone a little more familiar with Underscore is able to investigate in time for 5.3, we can move it back. But, it's possible that an upstream PR may be needed.

#10 @desrosj
5 years ago

  • Owner desrosj deleted
  • Status changed from reopened to assigned

#11 @TimoTijhof
4 years ago

I've taken a brief look at this, and don't see any link between Underscore and QUnit here. (Note that the upstream test suite for Undescore.js itself also uses QUnit, which is passing.)

Neither of QUnit nor Underscore modifies setTimeout, however. I do notice the error above mentions Sinon and sinon-qunit, which do modify the setTimeout reference globally. I suspect that might be what's causing an issue here.

Last edited 4 years ago by TimoTijhof (previous) (diff)

#12 @TimoTijhof
4 years ago

I've updated underscore locally to 1.9.1 and re-ran npm && npm run grunt travis:js which (I think) fetches the new library, copies it in-place, and runs the tests.

It did not cause a failure, however. Perhaps other changes or updates have since fixed the issue? Or perhaps I've not followed all the steps needed to reproduce it...

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


4 years ago

#14 @Hareesh Pillai
4 years ago

  • Keywords needs-patch added; has-patch removed
  • Summary changed from Update to Underscore 1.9.1 to Update Underscore to the latest version

Underscore has had many updates since then. The latest version is v1.12.0.

Changes between the latest version and the version bundled in Core can be viewed on Github.

Updating the ticket summary to reflect this.

#15 @Hareesh Pillai
4 years ago

  • Milestone changed from Future Release to 5.8

Seems like a good candidate for 5.8.
Adding it to the milestone.

#16 follow-up: @SergeyBiryukov
4 years ago

  • Keywords has-patch added; needs-patch removed

45785.3.diff is an initial refreshed patch that updates Underscore to version 1.13.0-1 and seems to work, but causes a failure in the verify:source-maps task during the build process (see [44740] / #46218 for more context):

Running "verify:source-maps" task
Warning: The build/wp-includes/js/underscore.js file must not contain a sourceMappingURL. Use --force to continue.

45785.4.diff fixes that by adding a new build task to remove the sourceMappingURL reference, though perhaps we'd want to make it more generic for future use, and not specific to underscore.js.

All tests seem to pass with 45785.4.diff.

List of changes in Underscore: https://github.com/jashkenas/underscore/compare/1.8.3...1.13.0-1

#17 in reply to: ↑ 16 @SergeyBiryukov
4 years ago

Replying to SergeyBiryukov:

45785.4.diff fixes that by adding a new build task to remove the sourceMappingURL reference, though perhaps we'd want to make it more generic for future use, and not specific to underscore.js.

45785.5.diff gives the task a more generic name so that it could be reused for other files.

#18 @davidbaumwald
4 years ago

  • Owner set to davidbaumwald
  • Status changed from assigned to accepted

#19 @davidbaumwald
4 years ago

  • Keywords commit added; needs-testing removed

Moving this to commit with @SergeyBiryukov's blessing.

#20 follow-up: @SergeyBiryukov
4 years ago

Just noting that when making the patch, I went to https://github.com/jashkenas/underscore/tags and 1.13.0-1 was the latest tag at the time, so I assumed it's the latest stable. Per discussion with @davidbaumwald, it was pointed out that the latest stable is actually 1.12.1, so let's go with that one for now and update to 1.13.0 when it's final.

#21 @davidbaumwald
4 years ago

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

In 50650:

External Libraries: Update Underscore to version 1.12.1.

Full set of changes at https://github.com/jashkenas/underscore/compare/1.8.3...1.12.1.
The new version includes a sourceMappingURL that causes a build failure, so this change
also introduces a task to remove this from the source during the build.

Props mukesh27, hareesh-pillai, desrosj, SergeyBiryukov, TimoTijhof.
Fixes #45785.

#22 in reply to: ↑ 20 @Hareesh Pillai
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to SergeyBiryukov:

Just noting that when making the patch, I went to https://github.com/jashkenas/underscore/tags and 1.13.0-1 was the latest tag at the time, so I assumed it's the latest stable. Per discussion with @davidbaumwald, it was pointed out that the latest stable is actually 1.12.1, so let's go with that one for now and update to 1.13.0 when it's final.

There is an update to Underscore (v 1.13.1) available now. We should update to that one.

#23 @Hareesh Pillai
3 years ago

  • Keywords needs-patch added; has-patch commit removed

This ticket was mentioned in PR #1203 on WordPress/wordpress-develop by dream-encode.


3 years ago
#24

  • Keywords has-patch added; needs-patch removed

#25 @davidbaumwald
3 years ago

In 50778:

External Libraries: Update Underscore to version 1.13.1.

A full set of changes can be found on GitHub: https://github.com/jashkenas/underscore/compare/1.12.1...1.13.1.

Follow-up to [50650].

Props hareesh-pillai.
See #45785.

#27 @Hareesh Pillai
3 years ago

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

#28 @ocean90
3 years ago

#53396 was marked as a duplicate.

Note: See TracTickets for help on using tickets.