fix(2767): prevent calling onRoute while prefixing

[salmanm]

Alternative fix for #2767

Approach explained in #2767 (comment)

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]

Maybe we could just skip https://github.com/fastify/fastify/pull/2782/files#diff-cf6996cafd1df0bc168a5c5f3a7470f11fc3aea9f908091d5dd6be2e7b30a2c3L200 if prefixing: true?

[salmanm]

Sorry the link doesn't seem to be taking me to a line I could relate your comment to.
What do you think we should skip?

[RafaelGSS]

I would recommend documenting this property here.

I also would like to add a note inside ignoreTrallingSlash documentation about this behavior under the hood.

[mcollina]

Sorry the link doesn't seem to be taking me to a line I could relate your comment to.
What do you think we should skip?

fastify/lib/route.js

Lines 200 to 207 in b8122e2

	for (const hook of this[kHooks].onRoute) {
	try {
	hook.call(this, opts)
	} catch (error) {
	done(error)
	return
	}
	}

[salmanm]

I see, so we don't call the onRoute hook twice in the first place. Makes sense. Updated the PR.

[mcollina]

lgtm

[mcollina]

Do you think this is semver-major or could we ship it in v3?

[salmanm]

This didn't cause any existing tests to change also the behavior isn't a documented or even expected one. So I'd not think of it as a semver-major.

[jsumners]

Do I understand this correctly? This PR adjust the internal API of route registration to pass around a new internal options object that does not mutate the user provided options object. This new internal object gets decorated with additional metadata about what sort of actions are being performed around route registration.

[salmanm]

@jsumners This PR does not change user provided options, neither we used to. Adding an extra metadata was my initial proposal before Matteo suggested we could skip triggering onRoute hook altogether while prefixing, which makes sense.

So right now, this PR only makes it skip calling onRoute for /foo/ when it already has triggered it for /foo.

[Eomm]

I agree to release it as semver-patch since the route object was the same 👍

[RafaelGSS]

LGTM.

[jsumners]

LGTM

[delvedor]

LGTM

[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.