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

fix: include directoryPrefix when using autoPrefix #350

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

KcZer0
Copy link

@KcZer0 KcZer0 commented Dec 28, 2023

I'm encountering this issue myself, so might as well fix it.

Fixes #205

Checklist

@KcZer0 KcZer0 deleted the branch fastify:master December 28, 2023 11:52
@KcZer0 KcZer0 closed this Dec 28, 2023
@KcZer0 KcZer0 deleted the master branch December 28, 2023 11:52
@KcZer0 KcZer0 restored the master branch December 28, 2023 11:52
@KcZer0 KcZer0 reopened this Dec 28, 2023
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@@ -69,7 +69,7 @@ app.ready(function (err) {
})

app.inject({
url: '/semiautomatic/items'
url: '/manualprefix/semiautomatic/items'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change necessary due to the change in the fixture, or is this a breaking change?

Can you please not alter existing tests?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry. Yes it is breaking change.

The behaviour between dirNameRoutePrefix and autoPrefix was not properly defined in the docs before this change.

Copy link
Author

@KcZer0 KcZer0 Dec 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the intended behaviour in the changed test is to override directoryPrefix, but keep options.prefix, I think a new exported option plugin.directoryOverride would be better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's avoid a breaking change, thanks.

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

Successfully merging this pull request may close these issues.

autoPrefix overwrites directory path set by dirNameRoutePrefix
2 participants