fix: onRoute hook should not be called when it registered after route#4052
fix: onRoute hook should not be called when it registered after route#4052
Conversation
|
Can you send the refactoring into its own PR? After merging that, this would be simpler to review |
|
can you rebase? |
1d2b8c4 to
d1cfda5
Compare
|
Rebased |
mcollina
left a comment
There was a problem hiding this comment.
lgtm, this is a good fix.
@fastify/fastify wdyt?
There was a problem hiding this comment.
I concur with @climba03003 that it doesn't need to be strictly synced. 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.
So, to simplify things. I go for 1. if we need to fix this issue. |
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? |
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();
The only thing which would be affected is when it call |
Eomm
left a comment
There was a problem hiding this comment.
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
|
The following code works at 4.1.0 const fastify = Fastify();
fastify.get('/', async () => '');
fastify.head('/', async () => '');At version 4.2.0, I got Is this expected behavior? |
|
Yes, it is.
from: https://github.com/fastify/fastify/blob/main/docs/Reference/Routes.md |
|
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. |
Fixes #4044
I don't think we should fix #4044, but I submit this PR for others to determine.
Checklist
npm run testandnpm run benchmarkand the Code of conduct