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

Add a redirected callback request option #97

Merged
merged 2 commits into from Jul 29, 2015
Merged

Conversation

@kanongil
Copy link
Member

kanongil commented Jul 3, 2015

Fixes #91 as suggested.

@geek

This comment has been minimized.

Copy link
Member

geek commented Jul 6, 2015

@kanongil I like this, but what are your thoughts on using the event emitter instead?

self.emit('redirect', statusCode, location, redirectReq);
@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Jul 6, 2015

since the callback can be called multiple times an event emitter seems more logical

@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented Jul 6, 2015

Event emitter on what exactly? The only exposed object is the ClientRequest from node.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Jul 6, 2015

A global one on Wreck doesn't make too much sense and changing stuff around so Wreck.post() returns an object you can listen on seems to big of a change for this(?), if the res from the callback is the ClientRequest you might add it to that?

@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented Jul 6, 2015

@AdriVanHoudt I would argue that it is very poor style to emit events on objects you don't own.

As far as I can tell, an option callback is the only viable approach.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Jul 6, 2015

@kanongil I would agree

@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented Jul 14, 2015

@geek Are there any other issues?

@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented Jul 29, 2015

Ping?

@geek geek added the feature label Jul 29, 2015
@geek geek self-assigned this Jul 29, 2015
@geek geek added this to the 6.0.1 milestone Jul 29, 2015
geek added a commit that referenced this pull request Jul 29, 2015
Add a redirected callback request option
@geek geek merged commit ee192df into hapijs:master Jul 29, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@geek

This comment has been minimized.

Copy link
Member

geek commented Jul 29, 2015

@kanongil sorry on the delay in getting this in, it looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.