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

Shouldn't request event emit request object? #209

Closed
tlivings opened this issue Nov 17, 2017 · 7 comments
Closed

Shouldn't request event emit request object? #209

tlivings opened this issue Nov 17, 2017 · 7 comments
Milestone

Comments

@tlivings
Copy link
Contributor

@tlivings tlivings commented Nov 17, 2017

I'd expect for the request event to include the created request object, not simply the uri and options (which also allows mutations of these options resulting in potential side effects).

Similar to the functionality of the response event.

This would allow for consistent instrumentation of both request and response objects (attaching event listenings for logging, etc).

Instead, the only way to get at it today is through the promise's req property which doesn't feel ideal (but probably workable).

@geek geek added the request label Nov 22, 2017
@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Nov 11, 2018

A year later, is this going to happen? If not please close.

@hueniverse hueniverse closed this Nov 11, 2018
@hueniverse hueniverse reopened this Nov 11, 2018
@tlivings

This comment has been minimized.

Copy link
Contributor Author

@tlivings tlivings commented Nov 16, 2018

I’ll open a PR. We worked around it but I think this would still make a lot of sense.

@tlivings

This comment has been minimized.

Copy link
Contributor Author

@tlivings tlivings commented Nov 21, 2018

It is a simple enbough change but there seems to be a desire, indicated by the testing, to allow mutation of the uri argument prior to creating the request.

If this is a still a requirement, then I'll introduce another event prior to the request creation, and modify the request event to pass the created request.

Thoughts?

@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Jan 20, 2019

@tlivings

This comment has been minimized.

Copy link
Contributor Author

@tlivings tlivings commented Jan 25, 2019

Ok I can just submit the PR this weekend and let @geek see what he thinks.

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Jan 25, 2019

@tlivings the problem is that the request isn't created until after this event fires (https://github.com/hapijs/wreck/blob/master/lib/index.js#L186-L191). Another option is to add a new event: onPostRequest (mirrored from the hapi event names). We probably will need to add pre events though if we go that route, which is probably a good direction to make it clear when the event fires.

tlivings added a commit to tlivings/wreck that referenced this issue Feb 8, 2019
tlivings added a commit to tlivings/wreck that referenced this issue Feb 8, 2019
@tlivings

This comment has been minimized.

Copy link
Contributor Author

@tlivings tlivings commented Feb 8, 2019

The above PR introduces a new preRequest event and changes the request event. If you'd like me to instead use a postRequest event to avoid the breaking change, let me know. This just seemed to be more semantically correct to me.

geek added a commit to geek/wreck that referenced this issue Mar 5, 2019
…nt now emits created request. hapijs#209

updated README
@geek geek added breaking changes and removed request labels Mar 5, 2019
@geek geek added this to the 15.0.0 milestone Mar 5, 2019
@geek geek closed this in #235 Mar 5, 2019
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.