feat: add minimal node-request

[imatlopez]

Description of change

The goal is to get rid of node-request. Initially I though of replacing the client with something lightweight like node-fetch, but the SHA validation require a streaming model which do not work will with promise-based http clients. So based on the input to node-request, I stripped out anything that doesn't have an effect like authentication which led to this very lightweight clone of the original module.

resolves #2047

cc @rvagg

Checklist

• npm install && npm test passes
• tests are included
• commit message follows commit guidelines

[imatlopez]

@rvagg @cclauss do y'all consider this semver-major? if so, what would make it semver-minor?

[rvagg]

btw there's also this discussion: npm/cli#1281 (comment) where Isaac suggested make-fetch-happen as an alternative - which would have one benefit of de-duping when bundled with npm. Although I haven't looked at the API and how difficult it might be. Once upon a time I did have a look at using the npm http client and decided it was too different to bother but I think that was pre make-fetch-happen. And the streaming+sha thing is a good (but not insurmountable) point.

Re semver-major, I think I'd be inclined to do so, not because we know there's breaking changes, but because this touches things that are likely to find breaking edges. But bumping to 8.0.0 isn't a big deal, I'd be happy to do that if this got merged.

[rvagg]

Sooooo upon reviewing it, this is a lot of code and looks like a separate package to me and maybe should just be published as its own thing. I'm not sure if it's worth going that far but this is a fair amount of code that we're taking a testing and maintenance burden for.

Also this:

I stripped out anything that doesn't have an effect like authentication

Is there a possibility that people are re-inserting authentication via environment variables that request might have received, like the proxy settings? I don't know because I don't use it but what if someone's doing an `export HTTP_PROXY=foo:[email protected]:8080", (a) does that even work with node-gyp and (b) would that break with this? I don't think we need a definitive answer to that, a release might shake out people for who these odd edge cases break, but it might be nice to catch as many of these corners as we can practically do.

[imatlopez]

Per https://github.com/request/request/blob/master/lib/auth.js it only handles authentication headers, url basic auth is not touched, which is nice.

Isn't the sha checking, a security feature? Probably best to not get rid of it.

with make-fetch-happen or even just node-fetch (minipass-fetch) proxies still need to be handled on their own. happy to try to make that change on a separate PR though code changes will be quite big in the install code since it will need to be switched to promises.

[rvagg]

Isn't the sha checking, a security feature? Probably best to not get rid of it.

yeah, not suggesting getting rid of it, just that it's not impossible to do it without handling it via the incoming stream, you can do it after the fact

with make-fetch-happen or even just node-fetch (minipass-fetch) proxies still need to be handled on their own.

I would hope that whatever npm is doing for proxies would cover what we're doing here. It's true that they download from different sources, but the people that need to proxy for node-gyp also need to proxy for npm. So maybe the delta isn't that big?

I just hate that we have to take on so much code burden to deal with this, http isn't supposed to be this complicated but we've built up so much technical debt here, in terms of code and features we afford users.

[rvagg]

btw it's not a NO to this PR, it's just not simple given the amount of code being introduced so I need time to form a stronger opinion, perhaps others want to weigh in.