Problem/Motivation

As part of #3094468: [plan] Update core JavaScript (and CSS) dependencies prior to 9.0.0-beta1 we need to update Underscore to 1.9.1 from 1.8.3

Proposed resolution

Update it

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#12 interdiff.txt59.82 KBlauriii
#8 interdiff.3101643.3-8.txt35.35 KBlongwave
#8 3101643-8.patch91.7 KBlongwave
#3 3101643-3.patch91.68 KBbnjmnm

Comments

bnjmnm created an issue. See original summary.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Parent issue: » #3094468: [plan] Update core JavaScript (and CSS) dependencies prior to 9.0.0-beta1
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Assigned: bnjmnm » Unassigned
Status: Active » Needs review
StatusFileSize
new 3101643-3.patch91.68 KB
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs review » Reviewed & tested by the community

This looks good. Did some manual testing with toolbar, ckeditor and quickedit on an image field which all use this library.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs review 
  1. +++ b/core/assets/vendor/underscore/underscore-min.js
@@ -1,6 +1,5 @@
\ No newline at end of file

    We should probably remove the newly added newline from the end. 

  2. +++ b/core/assets/vendor/underscore/underscore-min.js
--- /dev/null
+++ b/core/assets/vendor/underscore/underscore-min.js.map

+++ b/core/assets/vendor/underscore/underscore-min.js.map
index 60a56877dc..0000000000
--- a/core/assets/vendor/underscore/underscore-min.map

--- a/core/assets/vendor/underscore/underscore-min.map
+++ /dev/null

    Why has this file been renamed?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

#5.1 This is exactly how the file ships from upstream, I don't think we should be modifying it. I was wrong, upstream doesn't have the newline, so we should fix this

#5.2 This was done upstream in the 1.9.0 release; I think using the .js.map extension means the file itself doesn't need to specify sourceMappingURL, it can be inferred just by adding .map to the URL.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review
StatusFileSize
new 3101643-8.patch91.7 KB
new interdiff.3101643.3-8.txt35.35 KB
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs review » Reviewed & tested by the community

@longwave thanks for the patch.

  1. I confirmed the fix in #8 by applying the patch in #8 and then copying the file from the underscore release via the command line. Nothing changed so the end of line is correct.
  2. I also doubled checked the filename, underscore-min.js.map, in the release

  • lauriii committed 88bdfdd on 9.0.x 
    Issue #3101643 by longwave, bnjmnm, tedbow, lauriii: Update Underscore....

  • lauriii committed 539bb18 on 8.9.x 
    Issue #3101643 by longwave, bnjmnm, tedbow, lauriii: Update Underscore....
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Version: 9.0.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed
StatusFileSize
new interdiff.txt59.82 KB

I checked the md5 checksums and it seemed like the map file didn't match because of an extra newline at the end of the file. I copied the file from the underscore library again.

Committed 88bdfdd and pushed to 9.0.x and 8.9.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.