Update to Fastify v4

[mcollina]

Due to the changes introduced in fastify/fastify#2954, we need to change how this plugin is registered from:

  const fastify = Fastify()
  fastify.register(compressPlugin, {
    global: true
  })
  fastify.get('/one', (request, reply) => {
    reply.send(json)
  })

to

  const fastify = Fastify()
  await fastify.register(compressPlugin, {
    global: true
  }) // we must use await or after now.
  fastify.get('/one', (request, reply) => {
    reply.send(json)
  })

The fundamental problem of the change is that in Fastify v4 routes are now defined synchronously while plugins are defined asynchronously. Therefore requiring the await as there is no library to look for the onRoute hook.

Note that users of nested plugins are not impacted, so the following will still work:

  const fastify = Fastify()
  fastify.register(compressPlugin, {
    global: true
  })
  fastify.register(async (fastify) => {
    fastify.get('/one', (request, reply) => {
      reply.send(json)
    })
  })

Are we ok with the change?

cc @metcoder95 @fastify/core @fastify/plugins

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[Eomm]

The fundamental problem of the change is that in Fastify v4, routes are now defined synchronously while plugins are defined asynchronously. Therefore requiring the await as there is no library to look for the onRoute hook.

I missed that change! 😱

Now the onRoute position is now like the onRegister one

const fastify = require('fastify')({ logger: true })
fastify.addHook('onRoute', function hook(routeOptions) {
  console.log('works')
})
fastify.get('/', async (request, reply) => {
  return { hello: 'world' }
})
fastify.addHook('onRoute', function hook(routeOptions) {
  console.log('does not work')
})
fastify.ready()

Are we ok with the change?

Are we discussing if the onRoute hooks should be async or not?

[mcollina]

Now the onRoute position is now like the onRegister one

Exactly

Are we discussing if the onRoute hooks should be async or not?

Yes, exactly

[Fdawgs]

I may have missed this in previous issues re v4, so apologies if this has already been stated, but are we moving to ES modules?
The changes from require to import in the readme have thrown me.

[mcollina]

No. I have changed to import because top-level-await is available only on esm. The snippets would be invalid otherwise. I can restore require or just remove it in favor of fastifyCompress

[jsumners]

I think this is okay. I'm really wondering if we should move to removing the callback api completely, but that's a radical idea.

We need to fully illustrate this breaking change in the release notes.

[delvedor]

LGTM with a nit.

I'm really wondering if we should move to removing the callback api completely, but that's a radical idea.

I agree, maybe deprecating them in v4 and completely remove in v5?

[delvedor]

I think here we can keep ≥3, as the plugin code doesn't change. It only changes how plugin users are registering it. But this code as is could still work in v3.x

Suggested change

	fastify: '4.x',
	fastify: '>=3.x',

[mcollina]

From past experiences, maintaining that is a nightmare. We need separate tests for v3 and v4. I prefer to bump the major so we can avoid any compatibility issues down the line.

[mcollina]

v4 is already too late. Let's focus on shipping with the current set of features.

[fox1t]

Maybe it will generate some confusion initially, but having fewer options might be more straightforward to the users in the long run.

[Eomm]

Now the onRoute position is now like the onRegister one

Exactly

Are we discussing if the onRoute hooks should be async or not?

Yes, exactly

talking about the onRoute hook, if we compare the async vs sync, I see only pros for the async version.
Meanwhile, the sync onRegister is a technical obligation, it is not clear to me the reasoning to implement the sync onRoute

My mistake, it actually works

[climba03003]

LGTM.

[Fdawgs]

Do we still need engines in package.json, or does that need updating to node 14.x?
Also the version in package.json wasn't updated with the v6.0.0 release?

[climba03003]

Do we still need engines in package.json, or does that need updating to node 14.x?

IMO, engines fields should be removed, it is hard to manage.

Also the version in package.json wasn't updated with the v6.0.0 release?

I think it just forgotten to push the related commit.

[mcollina]

Fixed the tag.

[metcoder95]

LGTM as well, thanks for bringing me in the loop. Sorry for the headache, now I think I didn't document the changes properly, sorry for that 🙁

I think this is okay. I'm really wondering if we should move to removing the callback api completely, but that's a radical idea.

We need to fully illustrate this breaking change in the release notes.

Agree