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

Added option to toggle automatic following redirects #166

Merged

Conversation

ruyadorno
Copy link
Contributor

Hi @floatdrop and @sindresorhus,

I'm opening this PR with the goal of illustrating what such an option as described on #165 would look like.

Please review at your own discretion.

Thanks in advance 👍

@knksmith57
Copy link
Contributor

+1

@sindresorhus
Copy link
Owner

After much thinking I'm 👍 on this. @floatdrop You ok with this?

@floatdrop
Copy link
Contributor

@sindresorhus how about merge this with maxRedirects option? For example -1 in maxRedirects will disable redirect following.

@sindresorhus
Copy link
Owner

@floatdrop We don't have a maxRedirects option. ?

@floatdrop
Copy link
Contributor

@sindresorhus yep, just an idea.

@sindresorhus
Copy link
Owner

I'm not a big fan of -1 input, and in that case it could just be 0, but still, I don't really see the need for a maxRedirects option, and since we have neither at the moment, I'd rather just expose a followRedirects option that is actually requested by multiple people.

@ruyadorno ruyadorno force-pushed the disable-follow-redirect-option branch from c5b0699 to b3cd961 Compare March 30, 2016 18:14
@ruyadorno
Copy link
Contributor Author

rebased

@RoobinGood
Copy link

@ruyadorno are you sure that making redirect a successful result for request is a good idea? As far as I know it is not usual behavior.

@ruyadorno
Copy link
Contributor Author

@RoobinGood that's a good point, I'm not really sure of anything 😊

It does make sense in the context of the original use case behind this idea, as described in #165 but at the end of the day I just want to access the original redirect HTTP response in case of a 300 error, I guess that could also be achieved by attaching the response to the thrown got.HTTPError object but then I'm even less sure that's a better option.

It's also worth noting that we're talking about an optional configuration, it won't change the way got behaves if you don't use the followRedirect config property.

@floatdrop
Copy link
Contributor

@sindresorhus @ruyadorno are we ready to merge this option?

@ruyadorno
Copy link
Contributor Author

👍 sure, it was a good point brought to the discussion by @RoobinGood but I still think the current implementation is better 😊 we're getting more people interested in having this option... let's merge this

@floatdrop floatdrop merged commit 82446c9 into sindresorhus:master Apr 6, 2016
@ruyadorno ruyadorno deleted the disable-follow-redirect-option branch April 6, 2016 18:37
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.

5 participants