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

Migrate to hapi 17, node 8 and async/await closes #568 #572

Merged
merged 1 commit into from Dec 21, 2017
Merged

Conversation

@corbinu
Copy link
Contributor

corbinu commented Dec 12, 2017

Migrates to hapi 17, node 8 and async/await

I removed wreck support as wreck no longer issues the events needed

I am still trying to solve an issue where the tests emit MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 end listeners added. Use emitter.setMaxListeners() to increase limit

Any comments and help would be appreciated

@corbinu corbinu force-pushed the corbinu:master branch from fad46ce to 1d7eb18 Dec 12, 2017

const req = request.raw.req;
const res = request.raw.res;

this.event = 'response';
this.timestamp = request.info.received;
this.id = request.id;
this.instance = request.connection.info.uri;
this.labels = request.connection.settings.labels;
this.instance = server.info.uri;

This comment has been minimized.

Copy link
@arb

arb Dec 12, 2017

Contributor

Is the server not exposed in the request anymore?

This comment has been minimized.

Copy link
@corbinu

corbinu Dec 13, 2017

Author Contributor

I followed the upgrade instructions in the release notes for this. I also just searched the API docs page for uri and only see it as ever being attached to the server

}

startOps(interval) {

this._ops && this._ops.start(interval);
}

start(callback) {
start() {

This comment has been minimized.

Copy link
@arb

arb Dec 12, 2017

Contributor

So since we are cutting ties with old versions of hapi and node, you can rewrite

const ctorArgs = spec.args ? spec.args.slice() : [];
ctorArgs.unshift(null);
Ctor = Ctor.bind.apply(Ctor, ctorArgs);
const stream = new Ctor();

to use rest parameters. Not sure if you were planning on a subsequent JavaScript updates PR or not.

Should be a one-liner with rest parameters.

This comment has been minimized.

Copy link
@corbinu

corbinu Dec 13, 2017

Author Contributor

Nice catch I also removed the use of arguments bellow that

@@ -187,9 +154,10 @@ class Monitor {
});
}

return callback();
return Promise.resolve();

This comment has been minimized.

Copy link
@arb

arb Dec 12, 2017

Contributor

Is this needed?

This comment has been minimized.

Copy link
@corbinu

corbinu Dec 13, 2017

Author Contributor

Honestly no as the callback that was there before wasn't needed the whole function is synchronous. However I assume it was there to avoid confusion. At first I made the function async but then I got this linting error
Line 75: require-await - Async method 'start' has no 'await' expression.

This comment has been minimized.

Copy link
@arb

arb Dec 15, 2017

Contributor

I mean can you just not return anything? As you said, this isn't even an async function.

This comment has been minimized.

Copy link
@corbinu

corbinu Dec 15, 2017

Author Contributor

Done


pkg: require('../package.json')
};
exports.pkg = require('../package.json');

This comment has been minimized.

Copy link
@arb

arb Dec 12, 2017

Contributor

Why this change?

This comment has been minimized.

Copy link
@corbinu

corbinu Dec 13, 2017

Author Contributor

The plugin format has changed without it you get this error

[1] "name" is required
      at Object.exports.apply (/root/node_modules/hapi/lib/config.js:22:10)
      at internals.Server.register (/root/node_modules/hapi/lib/server.js:384:31)
      at it (/root/test/index.js:70:22)
      at Immediate.setImmediate (/root/node_modules/lab/lib/runner.js:566:31)
      at runCallback (timers.js:800:20)
      at tryOnImmediate (timers.js:762:5)
      at processImmediate [as _immediateCallback] (timers.js:733:5)

This comment has been minimized.

Copy link
@frankthelen

frankthelen Dec 14, 2017

Hmmm, you're sure that this works? According to the error message you have to provide exports.name in addition.

This comment has been minimized.

Copy link
@corbinu

corbinu Dec 14, 2017

Author Contributor

@frankthelen Yes I am sure the error was provided simply by switching it back and running the test suit to see it fail. pkg can be used as well please see the very last paragraph and code example here: https://hapijs.com/api

This comment has been minimized.

Copy link
@frankthelen

frankthelen Dec 15, 2017

Cool. You're right.

@corbinu corbinu force-pushed the corbinu:master branch from 1d7eb18 to 551e136 Dec 13, 2017
Copy link
Contributor Author

corbinu left a comment

Comments have been fixed or replied to. Let me know if I need to do anything else. Thanks so much!

@@ -187,9 +154,10 @@ class Monitor {
});
}

return callback();
return Promise.resolve();

This comment has been minimized.

Copy link
@corbinu

corbinu Dec 13, 2017

Author Contributor

Honestly no as the callback that was there before wasn't needed the whole function is synchronous. However I assume it was there to avoid confusion. At first I made the function async but then I got this linting error
Line 75: require-await - Async method 'start' has no 'await' expression.


const req = request.raw.req;
const res = request.raw.res;

this.event = 'response';
this.timestamp = request.info.received;
this.id = request.id;
this.instance = request.connection.info.uri;
this.labels = request.connection.settings.labels;
this.instance = server.info.uri;

This comment has been minimized.

Copy link
@corbinu

corbinu Dec 13, 2017

Author Contributor

I followed the upgrade instructions in the release notes for this. I also just searched the API docs page for uri and only see it as ever being attached to the server

}

startOps(interval) {

this._ops && this._ops.start(interval);
}

start(callback) {
start() {

This comment has been minimized.

Copy link
@corbinu

corbinu Dec 13, 2017

Author Contributor

Nice catch I also removed the use of arguments bellow that


pkg: require('../package.json')
};
exports.pkg = require('../package.json');

This comment has been minimized.

Copy link
@corbinu

corbinu Dec 13, 2017

Author Contributor

The plugin format has changed without it you get this error

[1] "name" is required
      at Object.exports.apply (/root/node_modules/hapi/lib/config.js:22:10)
      at internals.Server.register (/root/node_modules/hapi/lib/server.js:384:31)
      at it (/root/test/index.js:70:22)
      at Immediate.setImmediate (/root/node_modules/lab/lib/runner.js:566:31)
      at runCallback (timers.js:800:20)
      at tryOnImmediate (timers.js:762:5)
      at processImmediate [as _immediateCallback] (timers.js:733:5)
@corbinu corbinu changed the title Migrate to hapi 17, node 8 and async/await Migrate to hapi 17, node 8 and async/await closes #568 Dec 14, 2017
@arb arb added this to the 8.0.0 milestone Dec 15, 2017
@arb arb self-assigned this Dec 15, 2017
});
console.info(`Server started at ${ server.info.uri }`);

This comment has been minimized.

Copy link
@arb

arb Dec 15, 2017

Contributor

We should add something about the supported versions of hapi somewhere in the readme.

This comment has been minimized.

Copy link
@corbinu

corbinu Dec 15, 2017

Author Contributor

What format do you want this in. I looked at all the other plugins in the hapi namespace and didn't see an others doing this as was just going to follow whatever format was there.

This comment has been minimized.

Copy link
@corbinu

corbinu Dec 19, 2017

Author Contributor

@arb Just wanted to check if you had a chance to think about this. Don't mean to rush know we all get busy just excited ;)

This comment has been minimized.

Copy link
@arb

arb Dec 19, 2017

Contributor

Put it high in the README. Doesn't matter to me what the format is, just so long as it's there so when people open issues complaining, it is documented.

This comment has been minimized.

Copy link
@corbinu

corbinu Dec 20, 2017

Author Contributor

@arb Done!

@@ -187,9 +154,10 @@ class Monitor {
});
}

return callback();
return Promise.resolve();

This comment has been minimized.

Copy link
@arb

arb Dec 15, 2017

Contributor

I mean can you just not return anything? As you said, this isn't even an async function.

@@ -207,8 +169,9 @@ class Monitor {

// Do a setImmediate here so that all the streams listening for "end" have a chance to run
// https://github.com/nodejs/node/blob/master/lib/_stream_readable.js#L894-L897
setImmediate(callback);
return new Promise((resolve) => setImmediate(() => resolve()));

This comment has been minimized.

Copy link
@arb

arb Dec 15, 2017

Contributor

Curious if we still need this?

If so you can write it as return new Promise((resolve) => setImmediate(resolve));

This comment has been minimized.

Copy link
@corbinu

corbinu Dec 15, 2017

Author Contributor

I don't think so so removing

@corbinu corbinu force-pushed the corbinu:master branch from 551e136 to 893eeb7 Dec 15, 2017
@corbinu corbinu force-pushed the corbinu:master branch from 893eeb7 to 44b0e21 Dec 20, 2017
@teddmason

This comment has been minimized.

Copy link

teddmason commented Dec 21, 2017

I've just gone through upgrading a couple of apps to Hapi 17 and have also upgraded our good logging to @corbinu 's fork, all looking good for us, so far, thank you

@arb
arb approved these changes Dec 21, 2017
@arb arb merged commit 9ec13aa into hapijs:master Dec 21, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arb

This comment has been minimized.

Copy link
Contributor

arb commented Dec 21, 2017

Thanks @corbinu for taking care of this migration!

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.