Start loading the nested plugins after the current function completes

[mcollina]

This resolves fastify/fastify#329, but it will be a major version for avvio.

It is impossible for us to retain the context of which plugin is loaded in the current scenario:

avvio.use(function first  (s, opts, next) {
  s.use(second)
  s.use(third)
})

As in this case, third will be loaded as a child of second, not as a child of first. This is typically not a problem, but it messes with the chaining of prefixes ({ prefix: '/path' }) in fastify.

This change makes the callbacks to use (register in fastify) completely irrelevant within the plugins, and it makes the following code not work anymore:

avvio.use(function first  (s, opts, next) {
  s.use(second, next)
})

Instead this must be replaced by:

avvio.use(function first  (s, opts, next) {
  s.use(second)
  next()
})

The callback to use is still supported, but probably is only needed as the top-level, and I think we should stop recommending users to use it (in the plugins readme and the docs).

Another PR will be needed in fastify after this is released.

cc @delvedor @StarpTech

(I have also added debug and a lot of debug points, because I am tired of adding those in via console.log every time there is an hard issue here).

The docs would need to updated.

[mcollina]

I would also like to pull in #28, as this change would simplify it greatly.

[delvedor]

Stunning work!
I think that with this change the api is even better than before :)

[delvedor]

" instead of (?

[delvedor]

The error is:
throw new Error('Impossible to load (${obj.name}) plugin because the parent (${current.name}) was already loaded')
Why here there is not the was?

[StarpTech]

I also like the change and it simplifies the api to use next on top level instead of in nested calls.

[delvedor]

I would also like to pull in #28, as this change would simplify it greatly.

👍

[mcollina]

@delvedor this includes #28 as well and it's ready to land. Can I get one last check? Are any more tests that we should add?

[delvedor]

ready

[delvedor]

Why did you removed [cb]?

[mcollina]

because it's very bad to use it anyway, but we still support it for backward compat.

[delvedor]

We could add a debug log here, something like debug('resolving promise', this.name).
Same few lines below: debug('promise resolved', this.name).

[delvedor]

Everything looks good!!

A test that will fail, and we should be able to detect it, is:

test('promise long resolve', (t) => {
  t.plan(1)
  const app = boot()
  var insidePromise = false

  new Promise((resolve, reject) => {
    setTimeout(() => {
      resolve()
    }, 500)
  })
    .then(() => {
      app.use((s, opts, done) => {
        insidePromise = true
        done()
      })
    })

  app.ready(function () {
    t.ok(insidePromise)
  })
})

If we can't detect it we should document that this behaviour is not supported.

[mcollina]

@delvedor that test will fail right now.

A change that we can do is in https://github.com/mcollina/avvio/blob/master/boot.js#L110, basically not reinitializing avvio if the root plugin has finished loading.

One more question.. should we add app.start(), and not make it autostarting?

[mcollina]

@delvedor I implemented detection for that condition, you might want to remove.

[delvedor]

LGTM

[delvedor]

One more question.. should we add app.start(), and not make it autostarting?

I think we should autostart.