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

Have needle follow redirects #361

Merged
merged 4 commits into from
Apr 12, 2018

Conversation

allenluce
Copy link
Contributor

Redirect following for binary installations broke with the switch to needle. This replaces the functionality.

Not having a redirect means (at least in my case) that pre-built binaries that are registered as part of a Github release cannot be downloaded.

This addresses #359

Redirect following for binary installations was broken with the switch
to needle (in 9a9089b). This replaces the functionality.
lib/install.js Outdated
@@ -105,7 +106,10 @@ function place_binary(from,to,opts,callback) {
});

req.on('response', function(res) {
if (res.statusCode !== 200) {
if (res.statusCode === 302) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably want to allow other 30x codes here too I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed it to check for a Location: header instead.

@springmeyer
Copy link
Contributor

Thanks for catching this regression. Before merging this we'll need some kind of test to make sure it is actually working + does not regress again. Could you add a test?

Allen Luce added 2 commits March 28, 2018 13:02
This will handle things needle thinks are redirects without having to
check a bunch of status codes.
This introduces nock as a means of mocking HTTP requests.
@allenluce
Copy link
Contributor Author

FYI, I've just now noticed the error and have been able to repro it (using Node 4 under OSX, Node 6 on my box seems to work fine). I'll be able to tackle this in a couple hours.

@royaltm
Copy link

royaltm commented Mar 29, 2018

It affects all node versions, all systems.

So it doesn't fail on non-OSX, non-node-4 platforms.
akloboucnik added a commit to blockcollider/bcnode that referenced this pull request Apr 5, 2018
@allenluce
Copy link
Contributor Author

Is there something keeping this from being merged?

@springmeyer springmeyer merged commit 0cac29f into mapbox:master Apr 12, 2018
@springmeyer
Copy link
Contributor

Thanks for this work, it is now included in node-pre-gyp@0.9.1

ramijarrar added a commit to fractal-code/meteor-azure-server-init that referenced this pull request Apr 24, 2018
hyj1991 pushed a commit to X-Profiler/node-pre-gyp that referenced this pull request Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants