Follow redirects #22
Follow redirects #22
Conversation
|
I've updated the travis file, which should fix the build failures. Do you mind pulling that into your branch and repushing? |
| this.message = 'Aborted after ' + redirectNumber + ' redirects'; | ||
| this.name = 'TooManyRedirectsError'; | ||
| } | ||
| util.inherits(TooManyRedirectsError, Error); |
There was a problem hiding this comment.
@DonutEspresso should these be using the makeConstructor from Restify Errors? Or just stick with the established pattern?
There was a problem hiding this comment.
Yes, we should definitely use makeConstructor for the new stuff. Don't worry about the existing constructors (unless you want to give it a shot!).
refactor make jscs happy stringclient redirect small refactoring include maxRedirects param add tests for HEAD make jscs happy
wagnerfrancisco
force-pushed
the
follow-3xx-redirects
branch
from
f25aa6f
to
e8e085f
Compare
| @@ -343,6 +379,8 @@ function HttpClient(options) { | |||
| this.requestTimeout = options.requestTimeout || false; | |||
| this.headers = options.headers || {}; | |||
| this.log = options.log; | |||
| this.followRedirects = options.followRedirects || false; | |||
| this.maxRedirects = options.maxRedirects || 5; | |||
There was a problem hiding this comment.
Can you add corresponding "assert.optional" calls at the top of this constructor, please?
|
Thanks! I have pulled and pushed again but it's not working on node 0.10, I'll debug to know why. |
|
Building was failing because I was not consuming data from response object on redirects. That means I was not reusing sockets on redirects and node 0.10 (luckily) has a low maxSockets limit - just 5. When I reached that limit it stopped working. I'm consuming response obj now and build is passing. :) |
| @@ -89,6 +95,15 @@ StringClient.prototype.read = function read(options, callback) { | |||
| } | |||
|
|
|||
| req.once('result', self.parse(req, callback)); | |||
| req.once('redirect', function (res) { | |||
| res.resume(); | |||
There was a problem hiding this comment.
I did not call it. I have called .resume to consume data from res object. I found this on node docs:
If no 'response' handler is added, then the response will be entirely discarded. However, if you add a 'response' event handler, then you must consume the data from the response object, either by calling response.read() whenever there is a 'readable' event, or by adding a 'data' handler, or by calling the .resume() method.
https://nodejs.org/dist/latest-v4.x/docs/api/http.html#http_class_http_clientrequest
Is this wrong?
|
Looks good Wagner, do you mind pulling master into your branch again to clean up the merge conflicts? |
Conflicts: lib/HttpClient.js
|
Not at all, done! I had to fix some stuff cause linting was failing in master. |
|
Hi @micahr, there isn't any tag after this merge. Is there a policy to release a new tag or we just forgot to do this? Thanks! |
|
I didn't do a release, was holding on to see if there were any other changes. I'll go ahead and cut a release today. |
|
I just published Restify Clients 1.2.0. Thanks for your work @wagnerfrancisco! |
|
@micahr thanks for your attention and support! |
This PR is related to this suggestion: #19
301 and 302 redirects works like 303 - they force a GET request instead of preserving the original HTTP method unless original method is HEAD. Although this behaviour is not specified in RFC7231, almost every client works like this, so I believe it's good to keep the same behaviour. 307 redirects preserve the original HTTP method.
We just follow redirects if the followRedirects option is truthy. It's also possible to specify the maxRedirects option - which limits the number of redirects to follow. maxRedirects default value is 5. If we exceed this limit, a TooManyRedirectsError is thrown.
Disclaimer: HttpClient just sends a redirect event - StringClient is responsible for executing the redirect actually. I believe it's not possible to transparently follow redirects on the HttpClient because users manipulate the request object in the callbacks (e.g. end the request or send data). The user callback probably won't work as expected if the HTTP method changes on the redirect (e.g. from POST to GET). If you see a solution to fully handle redirects on HttpClient please let me know!