WordPress.org

Make WordPress Core

Opened 2 months ago

Closed 2 months ago

Last modified 2 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:

Description

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 2 months ago.
47257.2.diff (13.6 KB) - added by desrosj 2 months ago.
47257.3.diff (68.7 KB) - added by desrosj 2 months ago.

Download all attachments as: .zip

Change History (13)

@desrosj
2 months ago

#1 @desrosj
2 months ago

  • Milestone changed from Awaiting Review to 5.2.1

#2 @aduth
2 months ago

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

@desrosj
2 months ago

#3 @desrosj
2 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
2 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).

@desrosj
2 months ago

#5 @desrosj
2 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.


2 months ago

#7 follow-up: @aduth
2 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
2 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
2 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
2 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.