Closed DonutEspresso closed 6 years ago
I also want to add that the implementations of the parse()
method leave a bit to be desired - it mixes the lower level http errors along with parsing errors, and is quite difficult to reason about. I think it's worth addressing at some point but a bit outside of the scope of this PR, so I've kept the logic mostly intact.
Finished adding the complete suite of tests.
Thanks all, I tried to address all comments with the latest commits. Just to be sure, are we all aligned on HttpClient not emitting the after
event? I think it makes sense to me but want to make sure I'm not missing anything.
Summarizing some offline conversations:
We'll support after
even on the HttpClient, but errors cannot be as fully fleshed out as JSONClient/StringClient since the HttpClient does not do any response parsing. It's on userland to augment the existing error. If new errors are created in the callback (e.g., parsing errors), the only way after
can get access to them is if it's tacked on via the request or response objects.
This PR is now ready for a final pass.
@DonutEspresso Latest changes look good to me! Thank you! 🙏
Thanks for the feedback all!
I thought this was going to be straight forward but it got messy much more quickly than I expected. This mostly had to do with a patchwork of how the request lifecycle was being handled. In short, this PR:
emitResult()
function call (It used to callreq.emit('result')
in various places)onResult()
handler in StringClient to listen to theresult
event, using that handler to then invokeparse()
to parse the response bodyafter
event.metrics
object on to the request viareq.getMetrics()
methodBecause HttpClient consumes the streams directly,
after
doesn't really seem to make sense there. Thus, it's implemented in only StringClient and JSONClient. The tests aren't anywhere close to being complete - but wanted to get a sanity check first from the peanut gallery.