fastify / avvio

Asynchronous bootstrapping of Node applications
Other
412 stars 39 forks source link

Fix backport event order in master #99

Open Eomm opened 4 years ago

Eomm commented 4 years ago

I'm merging master in next, and the alpha-2 showed this issue

This script:

const avvio = require('.')()

avvio.ready().then(() => {
  console.log('ready')
})

// avvio.ready(() => {
//   console.log('ready')
// })

avvio.on('start', () => {
  console.log('start')
})

Produce:

Branch Promise Callback
next ready then start ready then start
master start then ready ready then start

Because of the change, this test is failing: https://github.com/fastify/fastify/blob/9b533f5062356e27d997eeee076375deba37cb69/test/decorator.test.js#L712

Because the fastify's server is flaged as started in the start event here: https://github.com/fastify/fastify/blob/9b533f5062356e27d997eeee076375deba37cb69/fastify.js#L287

I think ready->start is correct execution order, but if I think in all my application's tests, await server.ready() is quite the standard 😭

mcollina commented 4 years ago

I would say that "start then ready" is definitely incorrect and not the intention. I'm happy to fix it in avvio master.

Overall, the implementation of that behavior is wrong :/. I would recommend to fix it in master as well.