Remove the dependency on npm APIs

[bongnv]

Description of the Change

Since npm@8 drops programmable APIs, this PR removes references to those APIs so apm can be upgraded to npm@8 and Node 16. Follow changes are made to make it happen:

• refactor all calls to get npm config to use apm.getSetting instead
• getSetting spawns npm config get to load config instead of using npm.load API
• addBuildEnvVars and addProxyToEnv are converted to asynchronous to support that

Alternate Designs

Benefits

• another blocker to npm@8 will be removed and it will allow to upgrade to Node 16 and further

Possible Drawbacks

• spawning a new npm process may have a performance impact compared to using npm API. However, this only impacts apm which shouldn't be noticeable to users.

Verification Process

I verified the change by running tests and my own atom build from this fork https://github.com/bongnv/atom/blob/master/apm/package.json#L9

Applicable Issues

Haven't noticed any issues yet.

[DeeDeeG]

I haven't been very active at this repo lately, but I wanted to acknowledge this PR.

I agree with the general approach as described in the PR body text. npm 8 does indeed remove the internal APIs, which apm had relied on for a while... So one does have to call npm 8 strictly as a CLI program. No real way around this if moving to newer npm.

(Newer npm gives newer node-gyp, which is a pretty good thing to have, IMO.)

We can probably safely stay on npm 6 until Node 14 goes end-of life on April 30th 2023. (See the NodeJS release scedule.) npm 6 is bundled with Node 14, and it is implicitly supported until Node 14 goes end-of-life, if I recall correctly.

So I personally might be inclined to stay on npm 6 for now... But newer node-gyp does importantly come with support for newer Visual Studio versions, so it may be worth upgrading sooner rather than later. After April 2023 it would be smart to not stay on an unsupported npm version.

I can't speak to the performance implications without trying this change, but it wouldn't be too hard to informally benchmark it. With apm being a CLI app that's not run super often, it should be fine. It is run in the background at Atom startup, IIRC, but there's not much that can be done to avoid the performance hit, if there is one. You have to call npm as a CLI app as of npm 8, no two ways around it that I can see.

I haven't tested the code, but nothing about it stood out to me as looking wrong after a quick look.

[icecream17]

with the recent pr, npm 8 is used so now this is needed?
(edit)

[DeeDeeG]

So @icecream17, just to clarify, PR #125 fixed our CI environment only. We only "have latest npm" in terms of using it to install this repo's dependencies with npm install during the automated tests.

apm bundles its own internal copy of npm, and this is still npm 6.x

The current PR is very useful, since npm 6 will be sort-of end-of-life (?) in the upcoming month of May, given Node 14.x becoming End-of-Life then. (Node 14 has been shipping with npm 6, so npm 6 is only implicitly still supported for that reason.) Besides that newer node-gyp would help us a lot, and npm 7/8/9 should be much faster than npm 6 in some important ways.

The current PR lets us call npm as a pure CLI app, not load its internals as a library. (Loading npm's internal libraries is disabled as of npm 8 and newer.) This way we should be un-blocked from upgrading apm's internal copy of npm from 6.x to whatever future version, pretty much.

EDIT: I misread your comment, those last two paragraphs you presumably knew about. I thought you said "not needed", but I see you said "now needed".

[icecream17]

Edit: Invalid comment because the only way to not return is error or infinite wait, stopping execution

[icecream17]

Cursorily, lgtm.
And thanks for the clarification DeeDeeG, the comment was more helpful than it may have seemed.