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 event for response and logging #77

Merged
merged 6 commits into from Mar 23, 2015
Merged

Add event for response and logging #77

merged 6 commits into from Mar 23, 2015

Conversation

@geek
Copy link
Member

geek commented Mar 13, 2015

Fixes #72

This depends on the fine work @danielb2 did in PR #74

@geek geek added the feature label Mar 13, 2015
@geek geek added this to the 5.3.0 milestone Mar 13, 2015
@geek geek changed the title Add event for logging Add event for response and logging Mar 13, 2015
@danielb2

This comment has been minimized.

Copy link

danielb2 commented Mar 14, 2015

Very nice approach Wyatt ;)

@geek

This comment has been minimized.

Copy link
Member Author

geek commented Mar 15, 2015

@hueniverse thoughts on this approach? Let the implementer decide on logging, but provide an event for all wreck responses?

README.md Outdated

#### `response`

The response event is always executed for any request that *wreck* makes. The handler should accept the following

This comment has been minimized.

Copy link
@hueniverse

hueniverse Mar 15, 2015

Member

s/executed/emitted

@@ -17,14 +18,17 @@ var Tap = require('./tap');
var internals = {};


exports.agents = {
module.exports = new Events.EventEmitter();

This comment has been minimized.

Copy link
@hueniverse

hueniverse Mar 15, 2015

Member

This is kinda hackish. Better to use normal prototype style and new an object and export it.

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Mar 15, 2015

I don't think this is a good idea in its current form. It would be impossible to know what's going on if wreck is shared (because of node_modules) among multiple clients. I am not sure what's the use case here but I would assume if a module uses wreck and wants events, it only wants events triggered by it. So maybe allow using the global instance but also to new another which does not share the same emitter? Or only support events when a new instance is created?

@geek

This comment has been minimized.

Copy link
Member Author

geek commented Mar 15, 2015

@hueniverse thanks for the feedback. I will incorporate your advice. The major use case is to allow for developers to log wreck requests/responses in one spot. This is in replace of #76

geek added 2 commits Mar 16, 2015
@geek

This comment has been minimized.

Copy link
Member Author

geek commented Mar 16, 2015

@hueniverse thoughts on the current PR? I want to wait and see if anyone has issues with a module wide event emitter before exporting the constructor for Client.

};


// Wrapper so that shortcut can be optimized with required params
internals.shortcutWrap = function (method, uri /* [options], callback */) {
internals.Client.prototype.shortcutWrap = function (method, uri /* [options], callback */) {

This comment has been minimized.

Copy link
@arb

arb Mar 16, 2015

Contributor

Maybe _shortcutWarp since this isn't a documented method?

This comment has been minimized.

Copy link
@danielb2

danielb2 Mar 16, 2015

that or make it internals and pass self?

geek added a commit that referenced this pull request Mar 23, 2015
Add event for response and logging
@geek geek merged commit fd9da1a into hapijs:master Mar 23, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@geek geek deleted the geek:event branch Mar 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.