Make compose callback always execute async

[davidjamesstone]

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 their next 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 #14

[csrl]

I'm thinking this would be considered a breaking change and require a major version bump. Opinions?

[devinivy]

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.

[davidjamesstone]

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

[devinivy]

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!

[davidjamesstone]

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 👍

[csrl]

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.

[lock]

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.