Closed davidjamesstone closed 8 years ago
I'm thinking this would be considered a breaking change and require a major version bump. Opinions?
I agree that it would be a breaking change. Given that it's accepted for hapi plugins to behave sync or async, it's pretty hard for me to say whether I agree with this change or not. I definitely agree that it's preferable for async-looking things to behave asynchronously. But a change here almost implies that server.register()
itself should be changed, which is hard for me to agree with outright.
I'd like to see server.register
change. This issue then goes away but that's probably much harder to change I guess, and has more implications.
FWIW, I think it's still a worthwhile (breaking) change for glue
.
Writing plugins has always feel async-ish to me, given you need to call next
ala expressjs.
...just my 2 cents
Worth adding, the hapi style guide is at least clear on the point that callbacks named next
are the only ones that may occur before the next tick: http://hapijs.com/styleguide#callback. That does mean that plugins themselves don't have to follow the next-tick rule. I suppose server.register()
would be the next in line to ensure the rule is followed. It's a toss-up for me, but I'm pretty accustomed to hapi's behavior here!
Thanks for that link, I hadn't seen it before and didn't know about the next/callback naming convention.
I guess you do get used to it though I'm not sure it's entirely clear to newcomers to hapi-land.
My concern for glue
is It means that code could break/behaviour change if an async plugin is introduced at a later date.
E.g. This test code is good until an async plugin is registered in the manifest, after which module.exports would not be initialized:
Glue.compose(manifest, options, (err, server) => {
if (!module.parent) {
server.start(() => {
console.log('Server started on port:', server.info.uri);
});
} else {
module.exports = server;
}
});
I see your labbable
addresses this 👍
I think the short term "fix" is to change 'callback' to 'next' in glue.compose.
Regardless of server.register behavior, if glue.compose is called with a manifest that has an empty registrations, it still needs to maintain async behavior if the compose parameter is to be called 'callback', if we are to abide by the style guide.
This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.
Let me start by saying thanks for Glue - it's a great, really useful module.
One issue I have is with the
compose
method. It has an optional callback which can be invoked sync or async depending if any plugins being registered are themselves async (they call theirnext
function asynchronously.)This behaviour can cause astonishment and releases Zalgo
Would you consider forcing async behaviour with a little
process.nextTick
? Apologies if this has already been discussed.Related to https://github.com/hapijs/glue/issues/14