support await after / await use

[davidmarkclements]

for fastify this will enable

async function plugin (fastify, opts) {
  await fastify.register(somePlugin)
  console.log('after somePlugin has registered')
  fastify.register(anotherPlugin)
}

as an alternative to

async function plugin (fastify, opts) {
  fastify.register(somePlugin)
  fastify.after(() => {
    console.log('after somePlugin has registered')
    fastify.register(anotherPlugin)
  }) 
}

[davidmarkclements]

CI is passing in all node version except for node 6. Does this need to be addressed or can we drop 6?

cc @mcollina @delvedor @jsumners

[mcollina]

Please target https://github.com/mcollina/avvio/tree/next instead and drop 6 and 8.

[davidmarkclements]

@mcollina done

[delvedor]

Great work!!
Can you add a more complex test that verifies what happens when you register multiple plugins, some await and some not?
For example:

await app.use(first)
// check decorated app
app.use(second)
// check that second has noot decorated the instance
await app.use(third => app.use(fourth))
// check that we have third and fourth decorators
await app.ready()
// check all

[davidmarkclements]

Great work!!
Can you add a more complex test that verifies what happens when you register multiple plugins, some await and some not?
For example:

await app.use(first)
// check decorated app
app.use(second)
// check that second has noot decorated the instance
await app.use(third => app.use(fourth))
// check that we have third and fourth decorators
await app.ready()
// check all

So the way this is currently implemented is the queue is processed when we hit an await, to put this example into after terms this is what currently happens:

app.use(first)
app.after(() => {
  // check decorated app
  app.use(second)
  app.use(async function third (app) { app.use(fourth) })
  // CANT CHECK HERE BECAUSE AWAIT WILL TRIGGER AN AFTER
  app.after(async () => {
     // check that second has not decorated the instance (SPOILER, IT HAS)
     // check that we have third and fourth decorators
     await app.ready()
     // check all
  })
})

Are we saying it's essential to ensure that non-awaited use aren't processed until the end of the plugin function? /cc @mcollina

[delvedor]

Are we saying it's essential to ensure that non-awaited use aren't processed until the end of the plugin function?

I would not say that is essential, but it's important to at least document what's happening here.
Basically awaiting a use is similar to calling ready multiple times? Does it load everything that has been added until that point?
I'm trying to understand how should we signal this new feature, and if (and how) it will break existing code :)

[mcollina]

Are we saying it's essential to ensure that non-awaited use aren't processed until the end of the plugin function?

Yes. Right now, plugins are run after the main function complete. I think we should retain this behavior.
We should not start processing of plugins automatically.

Basically awaiting a use is similar to calling ready multiple times? Does it load everything that has been added until that point?

I think that should be the goal, yes.

[davidmarkclements]

@mcollina wait.. am I misunderstanding or is that a contradiction.

• non-awaited plugins should run after main function
• loading everything added until that point (the point where an await occurs)

I think I agree it's worth the effort to keep the behaviour as unsurprising as possible, which means processing unawaited plugins at the end of the function

[delvedor]

We have two options:
Option 1

app.use(first)
// app.firstDecorator = undefined

await app.use(second)
// app.firstDecorator = true
// app.secondDecorator = true

app.use(third)
// app.thirdDecorator = undefined

await app.ready()
// app.firstDecorator = true
// app.secondDecorator = true
// app.thirdDecorator = true

Option 2

app.use(first)
// app.firstDecorator = undefined

await app.use(second)
// app.firstDecorator = undefined
// app.secondDecorator = true

app.use(third)
// app.thirdDecorator = undefined

await app.ready()
// app.firstDecorator = true
// app.secondDecorator = true
// app.thirdDecorator = true

Which one do you think we should pick?
The option 2 is the one that does not introduce breaking changes with the current behavior.

[davidmarkclements]

the non-breaking changes one (2) - it's just a case of making it work

[mcollina]

Ahum, I think option 1 is better and it's compatible to how .after() wroks right now.

[davidmarkclements]

well @mcollina that depends on how you interpret the use of await, if you think of it as enclosing all code after the await in an after cb then yes it is, but if you think of it enclosing only that which is awaited in after then its more like 2

[delvedor]

Ahum, I think option 1 is better and it's compatible to how .after() wroks right now.

Yes, it's similar on when you will access the decorators, but the moment when the plugin is executed is not.

[mcollina]

Yes, it's similar on when you will access the decorators, but the moment when the plugin is executed is not.

in 1), the first plugin is run together with second when we have an after/await. In 2), the first plugin is run when .ready() is called. However first was defined before second. I'm not convinced that 2) is intuitive.

[davidmarkclements]

In the context of replicating after 1 may be more intuitive, but in the context of thinking about a queue + await:

queueAThing()
await aThing()
queueAThing()
await processTheQueue()

what's intuitive in this case? that both queueAThing ops run on await processTheQueue or that the first queueAThing op runs before aThing because there's an await? (it's the first right..)

[mcollina]

The problem is that await is external. consider this case:

app.use(first)
const p = app.use(second)
app.use(third)
await p // which one will be loaded?
await app.ready()

Note that the order in which plugins are loaded in scenario 2 depends on when .then() is called. I prefer a more deterministic order based on the order of calls to .use()

[davidmarkclements]

In your example under 2, it'll be second, then first, then third. Under 1 it'll be first second and third, but I think that's unintuitive. If you assign to p and await p somewhere else you emphatically want p (second) to load at the point you await it

[davidmarkclements]

@mcollina @delvedor I've added the more complex test (and some bug fixes) - behaviour is as per 1, tests show behaviour is as per 1.

If we decide on 1 this is probably ready to go, if we decide on 2 (which I'm actually more in favour of) then I'll adapt it for that. My vote is for 2, if you @delvedor @jsumners and @mcollina can come to a consensus I'll act accordingly.

[jsumners]

I think it's fair to say that if you do not await aThing() then you do not have any guarantee aThing() will be processed before doing anotherThing(). Nor should await anotherThing() have any dependency on aThing() completing before it can return with its result. Which is to say, option 2 makes the most sense.

[davidmarkclements]

thanks @Ethan-Arrowood for doing that!

ok I'm confused by this test, it's my understanding that after should run after all registers in the current tick.

In the failing test we're doing an after, then a decorate. Then as a sibling to the after, we're doing a register and expecting the decorator to be present? I'm not trying to be arrogant but is it possible that this test is incorrect and there is a bug in current? Or am I failing to understand how after is supposed to work?

[mcollina]

ok I'm confused by this test, it's my understanding that after should run after all registers in the current tick

That is not correct with the behavior in master. Each function passed to after is queued in the same queue of the plugins, and as a result it should be executed before the second register.

[davidmarkclements]

gotcha, okay

[mcollina]

you are missing reject.

[davidmarkclements]

yeah and a catch method. Promise returned from _loadRegistered has no rejection in its still currently a mystery to me on how to determine the failure mode of this case. A throw in a plugin that has been await registered does not end up on the _error property. Any tips appreciated

[mcollina]

lgtm

[mcollina]

@delvedor @Eomm @jsumners would you mind doing a final check before I land?

[jsumners]

LGTM

[Eomm]

I would share this case because I didn't understand what it should happen reading the README:

Without autostart the await app.after is blocking the loading:

const boot = require('..')
const app = {}
boot(app, { autostart: false })

async function run () {
  app.use((f, opts, cb) => {
    console.log('plugin init')
    app.use((f, opts, cb) => {
      console.log('plugin2 init')
      cb()
    })
    cb()
  })

  await app.after()
  console.log('after')

  await app.ready()
  console.log('ready')
}
run().catch(e => console.log(e))

Maybe should we threat await app.after as a trigger for starting the app as app.ready or avoid this setup?

[mcollina]

I think we should start. Il giorno mar 11 feb 2020 alle ore 22:37 Manuel Spigolon < [email protected]> ha scritto:

…

***@***.**** commented on this pull request. I would share this case because I didn't understand what it should happen reading the README: Without autostart the await app.after is blocking the loading: const boot = require('..')const app = {}boot(app, { autostart: false }) async function run () { app.use((f, opts, cb) => { console.log('plugin init') app.use((f, opts, cb) => { console.log('plugin2 init') cb() }) cb() }) await app.after() console.log('after') await app.ready() console.log('ready') }run().catch(e => console.log(e)) Maybe should we threat await app.after as a trigger for starting the app as app.ready or avoid this setup? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#92>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAMXY5TLW7NF4CYTFZKHVDRCMLCVANCNFSM4KEY7NQQ> .

[Eomm]

Oh, and I forgot to tell that, in the example, if you replace ready with start and autoload:true, an error is thrown

[mcollina]

I'm working on a fix for the problems you flagged @Eomm and one more that we have if override is used. I'll send a PR to Dave's branch.

[mcollina]

@Eomm this should now work as expected, thanks for the review.

[delvedor]

LGTM

[Ethan-Arrowood]

Great docs. Really cool code too! Nice work everyone. Also, this is very exciting because it unblocks a couple of things upstream in fastify 😄

[davidmarkclements]

ok I've had one last sweep and addressed nits. LGTM to me from here - cc @mcollina

[Eomm]

👏🏼