Callback and response event do not correspond to original

[RubenVerborgh]

The description states that follow-redirects is a "drop-in replacement". However, the replacement is not complete, as the response event is not correctly emitted.

In the original http and https implementations, the callback is a) optional and b) a shortcut for attaching a listener to the response event. However, follow-redirects makes the callback mandatory and does not reuse the response event. Any callbacks attached to response will fire at the wrong time (at the first request).

I realize this is hard to implement correctly, given that EventEmitter has no support for stopping event propagation. So if this cannot be fixed, it should probably be advertised that this module is not an exact replacement, but rather explain which parts it emulates and which parts it does not.

[jamestalmage]

It should probably follow a sensible pattern for any events that it can: https://nodejs.org/api/http.html

The list includes:

• abort
• checkExpectation
• connect
• continue
• response
• socket
• upgrade

For many of the above events, I suspect there is not a logical way to handle them that keeps redirection transparent. I have not explored enough to know the right answer for each.

My main goal with follow redirects, is allowing you to adapt your server side code to be reusable in the browser. Browsers handle redirects transparently, so any redirect handling code is wasted in the browser (and can break in the browser if you explicitly rely on redirection). From the docs:

follow-redirects provides a great solution for making the native node modules behave the same as they do in browserified builds in the browser. To avoid bundling unnecessary code you should tell browserify to swap out follow-redirects with the standard modules when bundling

All that said, I think correctly handling the response event is a good start (I am pretty sure that fits the browserify goal). It probably is worth examining how browserify-http and friends handle those events.

[RubenVerborgh]

The problem regarding the events you listed is: for which of the requests should they be handled? Because for a chain of n redirects, there are n + 1 requests.

For the response event, it makes sense: just for the last redirect. But for abort, for instance, should it be raised for the first, the last, or all? Brings me to another problem BTW, that a call to abort() should probably abort everything in the entire chain.

As far as browerify is concerned, it simply does not offer most of the events.

[jamestalmage]

The problem regarding the events you listed is: for which of the requests should they be handled? Because for a chain of n redirects, there are n requests.

Agreed. If we can't make sense of it. Just don't emit it.

But for abort, for instance, should it be raised for the first, the last, or all

abort is only raised if you call abort() right? If so, see below:

Brings me to another problem BTW, that a call to abort() should probably abort everything in the entire chain.

Agreed. Really, you don't have multiple connections open at once though. You receive a redirection event, close that connection and move on. This is probably just a matter of calling abort() on the latest, and shutting down redirection.

As far as browerify is concerned, it simply does not offer most of the events.

In that case, let's just not emit them.

[jamestalmage]

@RubenVerborgh - Do you feel #31 fully covers this issue, or are there more events that need work?

[RubenVerborgh]

#31 covers the response event, which was my reason for creating this issue #30. However, some additional things were brought up here, such as abort. They still need to be tackled, in this issue or another.

[jamestalmage]

Closing in favor of #34 which refocuses on the remaining question about abort()