Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update node-sass #15613

Merged
merged 1 commit into from
May 15, 2019
Merged

Update node-sass #15613

merged 1 commit into from
May 15, 2019

Conversation

desrosj
Copy link
Contributor

@desrosj desrosj commented May 13, 2019

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

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

@desrosj
Copy link
Contributor Author

desrosj commented May 13, 2019

Opened an accompanying ticket for Core.

@aduth aduth self-requested a review May 14, 2019 14:24
@aduth aduth added the Framework Issues related to broader framework topics, especially as it relates to javascript label May 14, 2019
@aduth
Copy link
Member

aduth commented May 14, 2019

This was discussed during today's #core-js chat (link requires registration):

https://wordpress.slack.com/archives/C5UNMSU4R/p1557843578353500

Noted there that technically the project doesn't support Node 12.x yet. That being said, this seems like a reasonable and non-conflicting change, with the added benefit of improved future compatibility.

You should be running a Node version matching the current active LTS release or newer for this plugin to work correctly.

https://github.com/WordPress/gutenberg/blob/master/docs/contributors/getting-started.md

12.x […] Active LTS Start: 2019-10-22

https://github.com/nodejs/Release#release-schedule

@swissspidy
Copy link
Member

There was similar discussion previously when I opened #12539 / #12541, but in the end it was merged without any issues. I don't see any downsides to this change now either.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍 Tested successfully both in Node 10.15.3 (active LTS) and Node 12.2.0.

@aduth aduth merged commit f340577 into master May 15, 2019
@aduth aduth deleted the update/node-sass-4-12 branch May 15, 2019 14:34
@gziolo gziolo added this to the 5.8 (Gutenberg) milestone May 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants