Make WordPress Core

Opened 21 months ago

Closed 21 months ago

Last modified 21 months ago

#47257 closed defect (bug) (fixed)

Upgrade node-sass to 4.12

Reported by: desrosj Owned by: desrosj
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch
Focuses: Cc:


Running npm install currently fails when running the latest version of Node (12.2.0 with NPM 6.9.0).

Core currently requires ~4.11.0 of node-sass, but only 4.12+ supports Node 12.2.0.

Attachments (3)

47257.diff (409 bytes) - added by desrosj 21 months ago.
47257.2.diff (13.6 KB) - added by desrosj 21 months ago.
47257.3.diff (68.7 KB) - added by desrosj 21 months ago.

Download all attachments as: .zip

Change History (13)

21 months ago

#1 @desrosj
21 months ago

  • Milestone changed from Awaiting Review to 5.2.1

#2 @aduth
21 months ago

From the proposed 47257.diff, should I expect it would include corresponding updates to the package-lock.json file as well?

21 months ago

#3 @desrosj
21 months ago

@aduth Ah, yes. Sorry, I missed including the package-lock.json changes in the previous patch. 47257.2.diff should have everything.

#4 @aduth
21 months ago

@desrosj Applying the revised patch and running npm install, I see that it produces a different result in package-lock.json.

Which versions of Node and NPM are you running? I'm not familiar with what's stipulated, but I've tried with both the `engines` versions (Node 10.13.0, npm 6.1.0) and Gutenberg recommendations (Node 10.15.3, npm 6.9.0).

21 months ago

#5 @desrosj
21 months ago

I am also seeing something different now when I try to reproduce 47257.2.diff.

I am using Node 10.15.3 (npm 6.4.1). I believe my steps were:

  • Change version to ~4.12.0.
  • Run npm install.

47257.3.diff has what I see now.

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

21 months ago

#7 follow-up: @aduth
21 months ago

I think 47257.2.diff is the most reasonable patch. Testing again (with latest NPM 6.9.0), I both see (a) no local changes after running npm install and (b) an identical patch when installing from 47257.diff (i.e. without the package-lock.json changes). The changes themselves also seem reasonable vs. 47257.3.diff where we shouldn't want to flip-flop between optional and dev properties of unaffected modules.

Related discussion in #core-js Slack earlier today, about specific NPM version to be using: https://wordpress.slack.com/archives/C5UNMSU4R/p1558022288363200

#8 in reply to: ↑ 7 @netweb
21 months ago

  • Keywords commit added

Replying to aduth:

I think 47257.2.diff is the most reasonable patch.

Agreed, 47257.2.diff looks good to me, untested, but still 👌

#9 @desrosj
21 months ago

  • Owner set to desrosj
  • Resolution set to fixed
  • Status changed from new to closed

In 45321:

Build/Test Tools: Upgrade node-sass to 4.12.0.

node-sass 4.12 adds support for Node 12. This will prevent errors for anyone running Node 12 locally instead of the LTS version (currently 10.15.3).

Props desrosj, aduth, netweb.
Fixes #47257.

#10 @desrosj
21 months ago

  • Keywords commit removed
  • Milestone changed from 5.2.1 to 5.3

I'm going to leave this in trunk (5.3) and not backport for now. It is not urgent. If there are more reports of users having issues testing 5.2, we can reconsider.

Note: See TracTickets for help on using tickets.