Revert "src: runtime deprecate process.umask()"

[BethGriggs]

This reverts commit 60c4c2b.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• documentation is changed or added
• commit message follows commit guidelines

---

Only the documentation deprecation was released in v14.x. We pushed the runtime deprecation to v15.x, but it looks like it will cause too much ecosystem disruption.

Refs: #35014 (comment)

Opening this PR to start the discussion on reverting the runtime deprecation.

[bnoordhuis]

If not now, then when? Pushing out the deprecation warning to v16.x, an LTS release line, will probably be more disruptive. With v15.x, there's at least a pretty big time window for library authors to update their code.

[targos]

@bnoordhuis the main problem right now is that this warning is printed everytime npm is executed.

[BridgeAR]

@ruyadorno could you have a look at npm 6 to fix this? That would be great :-)

[Trott]

@ruyadorno could you have a look at npm 6 to fix this? That would be great :-)

If I'm not mistaken, the problem is in the fs-minipass dependency and has been fixed since fs-minipass@2.0.1 but that npm depends on an older version due to its dependency on tar.

npm@6.14.8 /Users/trott/cli
└─┬ tar@4.4.13
  └── fs-minipass@1.2.7 

[Trott]

@ruyadorno could you have a look at npm 6 to fix this? That would be great :-)

If I'm not mistaken, the problem is in the fs-minipass dependency and has been fixed since fs-minipass@2.0.1 but that npm depends on an older version due to its dependency on tar.

npm@6.14.8 /Users/trott/cli
└─┬ tar@4.4.13
  └── fs-minipass@1.2.7 

And it looks like updating to tar@5.0.2 or later would solve the issue.

[Trott]

@ruyadorno could you have a look at npm 6 to fix this? That would be great :-)

If I'm not mistaken, the problem is in the fs-minipass dependency and has been fixed since fs-minipass@2.0.1 but that npm depends on an older version due to its dependency on tar.

npm@6.14.8 /Users/trott/cli
└─┬ tar@4.4.13
  └── fs-minipass@1.2.7 

And it looks like updating to tar@5.0.2 or later would solve the issue.

Looks like there's an existing issue in the npm issue tracker. I tried updating to tar@6 and all the tests still passed, so I'll submit a PR and move conversation over to that issue.

[AviVahl]

So you consider reverting a breaking change in 15.0.0 just because npm latest is trying to support an EOL version of Node (6.x)? :(

[Trott]

So you consider reverting a breaking change in 15.0.0 just because npm latest is trying to support an EOL version of Node (6.x)? :(

Not exactly "just because" that, no. We're considering delaying an unnecessary (albeit desirable) breaking change that will cause distress for the millions of npm 6.x users over something that the users will be powerless to do anything about. Given the choice between "Show millions of people warnings that they can't do anything about on nearly every npm invocation" and "Delay an optional breaking change", I'll go with delaying the optional breaking change.

We can re-land the breaking change for Node.js 16.x or 17.x and, if necessary and appropriate, work in the meantime to help npm move away from the deprecated API. Again, this seems like an easy call.

As for npm supporting EOL versions of Node.js, I have mixed feelings about that, and I won't say you're wrong to be frustrated by it. As I understand it, npm's policy is to support the oldest Node.js release that shipped with a major version of npm. Part of me wishes they wouldn't do that, but we're not going to be able to do anything about that in the immediate term. And I'm reluctant to tell someone with 10 million users (and a ton of data about the Node.js versions they're using ) that I know better than they do what technologies they need to support.

[ruyadorno]

Not exactly "just because" that, no. We're considering delaying an unnecessary (albeit desirable) breaking change that will cause distress for the millions of npm 6.x users over something that the users will be powerless to do anything about. Given the choice between "Show millions of people warnings that they can't do anything about on nearly every npm invocation" and "Delay an optional breaking change", I'll go with delaying the optional breaking change.

To build on top of @Trott reply, the way I understand it the last discussion in the Release WG came to the conclusion that having the warning being present in the npm version that is being shipped with core is more of a flagrant example that the change might be too disruptive to the ecosystem at this point in time.

@ruyadorno could you have a look at npm 6 to fix this? That would be great :-)

Given the above + the fact that it seems to be really last minute to try and patch it (I'm not fully aware of what it implies and last time I chatted with @isaacs about it, it seemed to be a non-trivial change) I'm in favor of reverting the change for v15 😊

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/33284/

[BethGriggs]

If not now, then when? Pushing out the deprecation warning to v16.x, an LTS release line, will probably be more disruptive.

Agreed, it would have been much better to go out in an odd release line. But I don't think that it is an option to release the runtime deprecation while npm emits the warning. As @Trott mentions, it will disrupt users who cannot really do anything about it.

Assuming there are no blocking objections, I'll move forward and land this. (Until this lands, CI is failing on v15.0.0-proposal with parallel/test-release-npm, and CITGM is harder to triage.)

[MylesBorins]

FWIW I had done some initial research into making the error log only when the call to umask was inside an application, rather than in a dependency (similar to what we did with buffer). This error is in C++ and the instantiation of process happens earlier in the bootstrap than any of the internal helpers we have for buffer. I think that could be a potential alternative solution, but have not figured out how to accomplish it within the constraints of process

[Trott]

Any chance npm 7 ships before Node.js 15 ships and we can include npm 7 and avoid this revert? Timeline seems awfully compressed for that, so I'm guessing the answer is "no" but you don't know if you don't ask, so I'm asking anyway.

[MylesBorins]

@Trott we are planning to get npm 7 into 15... but the umask warning is not fixed in npm 7 yet (to the best of my knowlege)

[Trott]

@Trott we are planning to get npm 7 into 15... but the umask warning is not fixed in npm 7 yet (to the best of my knowlege)

I'm using npm 7 with NODE_PENDING_DEPRECATION=1 set in my environment variables and not seeing the umask warning (or if it is showing up, at least it's not on every single command like it is in npm 6).

I am seeing this deprecation on every command though, due to (I think) undefined or null being assigned to a process.env property. I guess that deprecation is going into 15.x as well?

[DEP0104] DeprecationWarning: Assigning any value other than a string, number, or boolean to a process.env property is deprecated. Please make sure to convert the value to a string before setting process.env with it.
    at /Users/trott/.nvm/versions/node/v14.13.0/lib/node_modules/npm/lib/npm.js

[richardlau]

@Trott we are planning to get npm 7 into 15... but the umask warning is not fixed in npm 7 yet (to the best of my knowlege)

I'm using npm 7 with NODE_PENDING_DEPRECATION=1 set in my environment variables and not seeing the umask warning (or if it is showing up, at least it's not on every single command like it is in npm 6).

I am seeing this deprecation on every command though, due to (I think) undefined or null being assigned to a process.env property. I guess that deprecation is going into 15.x as well?

[DEP0104] DeprecationWarning: Assigning any value other than a string, number, or boolean to a process.env property is deprecated. Please make sure to convert the value to a string before setting process.env with it.
    at /Users/trott/.nvm/versions/node/v14.13.0/lib/node_modules/npm/lib/npm.js

@Trott You won't see the umask warning with Node.js 14.13.0 as it was only docs deprecated there.

[Trott]

@Trott You won't see the umask warning with Node.js 14.13.0 as it was only docs deprecated there.

I was going to write that I will see it because I have NODE_PENDING_DEPRECATION set in my environment. But now I realize I'm likely confusing the process.binding() runtime deprecation with the process.umask() runtime deprecation. 🙃 Sorry for the noise/confusion.

[MylesBorins]

Landed in 4acab22