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 Wreck support for version 11 and above #550

Merged
merged 3 commits into from May 17, 2017
Merged

Conversation

@lostthetrail
Copy link
Contributor

lostthetrail commented May 16, 2017

Resolves #549

@@ -1018,7 +1027,7 @@ describe('Monitor', () => {

expect(res.statusCode).to.equal(200);
// account for hapi15 differences
expect(out.data.length).to.be.greaterThan(0);
expect(out.data.length).to.be.greaterThan(1);

This comment has been minimized.

Copy link
@renegare

renegare May 17, 2017

@lostthetrail I think this should be set to least. However; thats just to make it pass. Not entirely sure why one can't assert one event given you only make one wreck request in this single test case 🤔

This comment has been minimized.

Copy link
@lostthetrail

lostthetrail May 17, 2017

Author Contributor

Drat. I had changed it from 0 to 1, when I was originally adjusting the unit test for wreck to confirm both cases (before I split it into two tests). I will revert that change in the next hour. Thanks!

@lostthetrail lostthetrail force-pushed the lostthetrail:master branch from ad6f32e to 6ec53a2 May 17, 2017
@lostthetrail lostthetrail force-pushed the lostthetrail:master branch from 6ec53a2 to 51e9285 May 17, 2017
@@ -68,6 +68,15 @@ class Monitor {
if (this.settings.wreck) {
this._wreckHandler = (error, request, response, start, uri) => {

// Wreck 11+ emits response event with signature (err, {req, res, start, uri})
const isWreckVersion11OrHigher = (!response && typeof request === 'object');

This comment has been minimized.

Copy link
@arb

arb May 17, 2017

Contributor

Thoughts on making this a proper function and checked arguments.length instead?

@arb

This comment has been minimized.

Copy link
Contributor

arb commented May 17, 2017

CI failing.

@arb arb added this to the 7.2.0 milestone May 17, 2017
@arb arb added the feature label May 17, 2017
@lostthetrail

This comment has been minimized.

Copy link
Contributor Author

lostthetrail commented May 17, 2017

@arb Since this was a bug fix more than a new feature enhancement, I changed as little as possible.

Would you prefer if I adjusted from arrow to function and use the arguments list to dynamically determine values?

@arb

This comment has been minimized.

Copy link
Contributor

arb commented May 17, 2017

Yeah I think so.

@lostthetrail

This comment has been minimized.

Copy link
Contributor Author

lostthetrail commented May 17, 2017

You got it. Standby.

@lostthetrail

This comment has been minimized.

Copy link
Contributor Author

lostthetrail commented May 17, 2017

@arb Back to you. Anything else you would like adjusted?

@arb arb merged commit 8cbda47 into hapijs:master May 17, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.