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 #7603

Closed
wants to merge 1 commit into from
Closed

Update Node SASS #7603

wants to merge 1 commit into from

Conversation

youknowriad
Copy link
Contributor

I was having 404 errors when running npm install.

Testing instructions

  • npm install && npm run dev
  • Check that Gutenberg is loading properly

@youknowriad youknowriad added the [Type] Build Tooling Issues or PRs related to build tooling label Jun 28, 2018
@youknowriad youknowriad self-assigned this Jun 28, 2018
@youknowriad youknowriad requested review from gziolo and ntwb June 28, 2018 11:04
@youknowriad
Copy link
Contributor Author

youknowriad commented Jun 28, 2018

Oops that's a big diff in package-lock. Checking again.

Edit: It looks legit.

@ntwb
Copy link
Member

ntwb commented Jun 28, 2018

Oops that's a big diff in package-lock. Checking again.

I had a similar issue yesterday, for some reason a bunch of "dev": true fields are being added, which I think is correct per the docs:

dev
"If true then this dependency is either a development dependency ONLY of the top level module or a transitive dependency of one. This is false for dependencies that are both a development dependency of the top level and a transitive dependency of a non-development dependency of the top level."

@ntwb
Copy link
Member

ntwb commented Jun 28, 2018

The reason we are using a pinned version of node-sass is in #6772

node-sass changes from v4.7.2 to v4.7.0, this is due to node-sass > v4.7.1 locking down the request library to ~2.79.0 which includes a vulnerable hoek package, see https://nodesecurity.io/advisories/566. It also uses the v4.7.0 release from GitHub rather than npmjs.com because v4.7.0 is not published to npmjs.com. This issue will not be resolved by node-sass until v5.0.0 is released in a few weeks time when as part of that release Node.js v4 support will be dropped.

Unfortuately node-sass v5 is still unreleased sass/node-sass#2312

@ntwb
Copy link
Member

ntwb commented Jun 28, 2018

@youknowriad can you try running npm rebuild node-sass using the existing v4.7.0 please?

@youknowriad
Copy link
Contributor Author

I'm fine with keeping the current version but I still get 404 when trying to run npm install

@ntwb
Copy link
Member

ntwb commented Jun 28, 2018

Does it display the filename of the 404 during this?

@youknowriad
Copy link
Contributor Author

youknowriad commented Jun 28, 2018

@ntwb
Copy link
Member

ntwb commented Jun 28, 2018

Right, yes, unfortunately, darwin-x64-64_binding.node is not listed at: https://github.com/sass/node-sass/releases/tag/v4.7.0

Just to confirm what node.js and npm version are you using @youknowriad ?

@youknowriad
Copy link
Contributor Author

Actually, this is not really needed. I'm closing, after further investigation, it looks like the dependency binaries are built after the failing 404 but in a special setup (trying to build a docker image) the build also fails so it's probably a specific failure.

@youknowriad youknowriad deleted the update/node-sass branch June 28, 2018 13:33
@ntwb
Copy link
Member

ntwb commented Jun 28, 2018

Thanks @youknowriad

p.s. @pento Gutenberg is not compatible with Node.js v10.x by way of node-sass incompatibilities with the 4.7.0 release we are currently pinned to.

We pinned to 4.7.0 to get npm audit to pass

The node-sass v4.9.0 release supports Node.js v10 but breaks npm audit

@pento
Copy link
Member

pento commented Jul 10, 2018

@ntwb: Apparently node-sass 4.9.1 fixes the npm audit noise. Let's upgrade it, then we can hopefully upgrade Node to v10.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants