Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

autohooks break prefixOverride #380

Closed
2 tasks done
rommelandrea opened this issue Jun 10, 2024 · 9 comments
Closed
2 tasks done

autohooks break prefixOverride #380

rommelandrea opened this issue Jun 10, 2024 · 9 comments

Comments

@rommelandrea
Copy link

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the bug has not already been reported

Fastify version

4.27.0

Plugin version

5.8.1+

Node.js version

20.12.2

Operating system

Linux

Operating system version (i.e. 20.04, 11.3, 10)

Linux

Description

From autoload 5.8.1 onward, if an autohook is defined and there is an override of the prefix defined in either hooks or route, the override will be ignored.

Link to code that reproduces the bug

https://github.com/rommelandrea/fastify-autoload-bug

Expected Behavior

when prefixOverride is exported the route change the prefix instead of use the default.

@mcollina
Copy link
Member

cc @climba03003 wdyt?

@climba03003
Copy link
Member

It is a complicated problem, I think we should

  1. Revert all changes related to composedPlugin should have its own prefix context #368
  2. Rework the files finding and tree building - it is the critical one because all the information is scattered currently.
  3. Re-land the feature in 1.

@mcollina
Copy link
Member

@jean-michelet wdyt?

@jean-michelet
Copy link
Contributor

I am gonna investigate, but climba probably knows the component better than I do so if I struggle to fix this issue I will push a revert PR as suggested.

@mcollina
Copy link
Member

Thanks @jean-michelet

@jean-michelet
Copy link
Contributor

I will do it tomorrow or Friday at the latest, sent you a DM on Discord @mcollina

@jean-michelet
Copy link
Contributor

https://github.com/rommelandrea/fastify-autoload-bug/blob/a2720e87a4c5d6fbfc6029868989774e525d1da8/routes/example/autohooks.js#L7
Autohooks plugins are managed as an independent tree without all these helpers @rommelandrea
Maybe we can add a note to the doc.

To reverse #368, I also need to reverse #374, if I understand correctly. Can you confirm before I push @climba03003?

At the moment, it's difficult to solve this issue... We should state in the documentation that setNotFoundHandler cannot be used in autohooks. If people really need this feature, they'll push a PR with tests. But I think I will take the time to refactor the whole component as suggested.

@jean-michelet
Copy link
Contributor

This test breaks when I revert:

@climba03003
Copy link
Member

@jean-michelet
You should revert #378, #375 and #368 in order

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants