[fix] node v10.0 lacks fs.promises
#2654
Conversation
|
I believe this is why npm7 doesn't run under node8 also, at least the biggest hurdle. Fixing this COULD have negative repercussions to people who currently are installing npm7 on node8, in that it could now break in new less visible ways. It may be worth discussing making sure that npm7 won't install itself outside its engine.node bounds before we fix this. |
|
@wraithgar that is easily avoidable by sticking some syntax in the npm cli entrypoint that breaks on node 8 - for example: /* npm requires node 10+ */ (async function* () {}) |
|
Ideally we do this in a way that you can't install at all, having an npm that refuses to run after it's installed is no different than having one that is broken after it's installed. And since we're already planning on solving the other issue, it could be better to do that first. #2612 |
|
I don't mind the order, but to me it seems far more important to ensure npm works on the engines it supports than to ensure it fails to install on the engines it doesn't support :-) |
|
Either way, the dependent PRs could presumably be landed sooner? |
This is a good point. Another thing that had been floated was updating the engines declaration since these v10 versions have security vulnerabilities associated with them. However the discussion around updating a semver minor in I'll +1 this but am still nervous about what npm is going to do in node8. We'd best get the other issue solved sooner than later also. |
|
yeah, it needs a test that simulates node<10 by mocking |
|
To be clear; per-job coverage checks will always fail in a project that has platform version fallbacks. Separately, this PR’s tests will fail until the dependent PRs are merged and released, and the deps are updated here. |
|
I am nervous about having checkmarks not be green on PRs, that really does train us to ignore CI tests. |
|
@wraithgar having them sit there red shouldn't be training anyone :-) only merging through a red x would do that. I'll mark this as a draft for now since the others aren't landed yet. |
ljharb
force-pushed
the
latest
branch
3 times, most recently
from
dc2ca45
to
5daf858
Compare
|
What do i do with the |
ljharb
force-pushed
the
latest
branch
2 times, most recently
from
2041b9c
to
a2aa1fa
Compare
Clobber |
|
OK, I've updated the doctor tests. Presumably these commits should remain separate when landing. I'll look at the snapshot test failures next. |
darcyclarke
added
Release 7.x
wraithgar
removed
the
release: next
|
Turns out node v10.0 has some really ghostly I'm bumping up the tests to 10.1, and will leave it to yall to bump the engines to 10.1 if that's the best outcome. |
|
@ruyadorno because npm uses licensee at the top level in its license validation, and it is a bug that it wasn't there in the first place. |
yeah cool, that sounds like a sep PR fix - will make things easier to manage
yeah I think specifically the |
or even better, we can just make sure to add it in the release branch, no need to send a PR |
|
right, but for this PR to pass, it has to be in the I'll put up separate PRs for the other 3 commits, and once those land, i can rebase this one down to the single commit. |
|
The dependency update is already in the upcoming release branch, so I went ahead and closed that one. I'll look at the other two and start pulling them in if they look good (since this PR was already very heavily vetted I assume it will be a very straightforward process) |
|
Ok the two PRs that aren't the |
|
In order for this PR to be rebased to the isolated changes, the other PRs need to be in the LATEST branch (the base branch for this PR). Being in the release branch has no impact on this PR. Can those PRs be landed in the latest branch? |
|
I am going to do some juggling here and see if I can clean this up real quick ETA, forgot this was a PR from a fork. Hrm. |
|
I can rebase it on top of the release branch if you'd like, this is just very bizarre to me :-) |
|
Done, this is now a single commit on top of the release branch. |
In this node version, fall back to `util.promisify` of the callback version. Maybe fixes npm#2623. Maybe fixes npm#2652. Maybe fixes npm#2625. PR-URL: npm#2654 Credit: @ljharb Close: npm#2654 Reviewed-by: @wraithgar, @ruyadorno
wraithgar
added
the
release: next
|
Aaand just so we can all see that things are working here in the release branch, with this PR included
(The first failure was an os/x hiccup) |
|
with v7.5.4, my node v10.0 CI builds are passing, yay! |
| const { promises: { unlink } } = require('fs') | ||
| const util = require('util') | ||
| const fs = require('fs') | ||
| const { unlink } = fs.promises || util.promisify(fs.unlink) |
There was a problem hiding this comment.
How does one destructure an unlink property from a promisified function value? It's not following the pattern of lib/utils/reify-finish.js (or any number of dependency updates to address missing fs.promises), so maybe the tests are secretly failing for shrinkwrap? "Not throwing" isn't the same as "correct behavior in alternate circumstance".
-const { unlink } = fs.promises || util.promisify(fs.unlink)
+const { unlink } = fs.promises || { unlink: util.promisify(fs.unlink) }There was a problem hiding this comment.
whoops, you're right. this was a mistake. i'll put up a quick PR to fix it.
There was a problem hiding this comment.
Note that tests aren't actually running in node 10.0, so the failures there wouldn't have shown up in CI.
See npm#2654 (comment) PR-URL: npm#2688 Credit: @ljharb Close: npm#2688 Reviewed-by: @wraithgar
In this node version, fall back to
util.promisifyof the callback version.Maybe fixes #2623. Maybe fixes #2652. Maybe fixes #2625.
Updated the tests to run on the
.0s as well.This is likely blocked until npm/node-gyp#5 and npm/run-script#21 are landed, released, and updated here.