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.

Sign up for GitHub

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

Jump to bottom

autoPrefix overwrites directory path set by dirNameRoutePrefix #205

Open
2 tasks done
mariusa opened this issue Nov 29, 2021 · 4 comments · May be fixed by #350
Open
2 tasks done

autoPrefix overwrites directory path set by dirNameRoutePrefix #205

mariusa opened this issue Nov 29, 2021 · 4 comments · May be fixed by #350
Labels
good first issue Good for newcomers

Comments

@mariusa
Copy link

mariusa commented Nov 29, 2021

Prerequisites

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

Issue

Hi,

Using the example from README

plugins
│   ├── hooked-plugin
│   │   ├── autohooks.js // req.hookOne = 'yes' # CJS syntax
│   │   ├── routes.js
│   │   └── children
│   │       ├── old-routes.js
│   │       ├── new-routes.js
│   │       └── grandchildren
│   │           ├── autohooks.mjs // req.hookTwo = 'yes' # ESM syntax
│   │           └── routes.mjs
│   └── standard-plugin
│       └── routes.js

Say new-routes.js has a route with fastify.patch('/entity', ...

That is available at http://.../hooked-plugin/children/entity , as by default dirNameRoutePrefix is set. So far so good.

In new-routes.js, I've set

export const autoPrefix = '/batch'

I would have expected the route to be now at http://.../hooked-plugin/children/batch/entity but instead is at
http://.../hooked-plugin/batch/entity
If this is intended behavior, would you please mention in README for autoPrefix that it overwrites, not appends, to folder structure path?
This isn't consistent with When setting both options.prefix and plugin.autoPrefix they will be concatenated., but at least it's documented.

Thanks!
@mcollina
Copy link
Member

mcollina commented Dec 4, 2021

Sorry for the radio silence. It seems no one is available to triage this. I'm going to tag this as "good first issue" to give some visibility.

(I encourage you to experiment and look into the code).

@mcollina mcollina added the good first issue Good for newcomers label Dec 4, 2021
@mattcasey
Copy link

mattcasey commented Dec 29, 2021 •
edited

Chiming in because I'm interested in 'fixing' this, but it seems like a considerable breaking change. The document is correct: when set, the prefix option passed to fastify.register() and the autoPrefix set in a plugin will be concatted. OP is asking that you also concat the prefix sourced from the directory name, which is currently overridden whenever autoPrefix is set: https://github.com/fastify/fastify-autoload/blob/master/index.js#L248.

I took a crack at it, but some tests are failing and I don't understand what the expected behavior will always be: https://github.com/fastify/fastify-autoload/pull/214/files.

@m4rvr
Copy link

m4rvr commented Nov 7, 2023

Any update on this?

@mcollina
Copy link
Member

Any update on this?

Would you like to send a Pull Request to address this issue? Remember to add unit tests.

@KcZer0 KcZer0 linked a pull request Dec 28, 2023 that will close this issue
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: include directoryPrefix when using autoPrefix KcZer0/fastify-autoload
4 participants
@mcollina @mattcasey @mariusa @m4rvr