fix: onRoute hook should not be called when it registered after route

[climba03003]

Fixes #4044

I don't think we should fix #4044, but I submit this PR for others to determine.

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

[mcollina]

Can you send the refactoring into its own PR? After merging that, this would be simpler to review

[mcollina]

can you rebase?

[climba03003]

Rebased

[mcollina]

lgtm, this is a good fix.

@fastify/fastify wdyt?

[RafaelGSS]

I concur with @climba03003 that it doesn't need to be strictly synced. Also, it might be a breaking change

[climba03003]

Also, it might be a breaking change

This fix is not breaking change and it also allow more usage like below one that should not works before.

const fastify = Fastify()
  
fastify.get('/', async () => '')
fastify.after(() => {
  fastify.head('/', async () => '') 
})

Any fix I can think of would provide side-effect in different way.

1. This approach, make the head route creation in sync and remove it when needed.

• It cause the route rebuild when remove the fastify route. (which can be mitigate by create head route before get.)
• Cold start time may be increase due to route rebuild.

2. Slice the current hook array and store it for later use.

• It cause the memory usage in cold start spark.
• We may not cache the correct hook array.

So, to simplify things. I go for 1. if we need to fix this issue.

[RafaelGSS]

This fix is not breaking change and it also allow more usage like below one that should not works before.

What about users using this approach:

function main (fastify, opts, next) {
  fastify.addHook('onRoute', () => {})
  fastify.register(allApplicationRoutes)

  next()
}

They will be affected, won't they?

[climba03003]

They will be affected, won't they?

I don't think it will be affected. Or you can describe more on what is the expected behavior you would like to see.

import Fastify from "fastify";
const fastify = Fastify();

fastify.addHook("onRoute", function (options) {
  console.log("onRoute", options.method);
});
fastify.register(function (instance, _, done) {
  instance.get("/", () => {});
  done();
});

await fastify.ready();

main and this PR should both print

onRoute GET
onRoute HEAD

The only thing which would be affected is when it call onRoute and it can be called onRoute multiple times for HEAD route.
We may mitigate the problem by exposing kRouteByFastify to the user.
They can be aware this route is generate by fastify.

[Eomm]

They can be aware this route is generate by fastify.

I think this is not useful for the user.

In this case we are saying that the hook is executed more times than the expected

[OpportunityLiu]

The following code works at 4.1.0

const fastify = Fastify();

fastify.get('/', async () => '');
fastify.head('/', async () => '');

At version 4.2.0, I got

FastifyError [Error]: Method 'HEAD' already declared for route '/'
    at Object.addNewRoute (...\node_modules\fastify\lib\route.js:269:19)
    at Object.route (...\node_modules\fastify\lib\route.js:196:19)
    at Object.prepareRoute (...\node_modules\fastify\lib\route.js:143:18)
    at Object._head [as head] (...\node_modules\fastify\fastify.js:247:34)
    at file:///.../t.js:6:9
    at ModuleJob.run (node:internal/modules/esm/module_job:198:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:385:24)
    at async loadESM (node:internal/process/esm_loader:88:5)
    at async handleMainPromise (node:internal/modules/run_main:61:12) {
  code: 'FST_ERR_DUPLICATED_ROUTE',
  statusCode: 500
}

Is this expected behavior?

[Eomm]

Yes, it is.
It was wrong that we were not throwing

exposeHeadRoute: creates a sibling HEAD route for any GET routes. Defaults to the value of exposeHeadRoutes instance option. If you want a custom HEAD handler without disabling this option, make sure to define it before the GET route.

from: https://github.com/fastify/fastify/blob/main/docs/Reference/Routes.md

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.