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: routes #166

Closed
wants to merge 1 commit into from
Closed

fix: routes #166

wants to merge 1 commit into from

Conversation

simoncarbajal
Copy link

This fixes the following issue:

Screenshot 2024-07-26 at 09 27 02

Checklist

@mcollina
Copy link
Member

@jdhollander can you take a look at this? I have a feeling this might undo some of your work.

@jdhollander
Copy link
Contributor

jdhollander commented Jul 27, 2024

@simoncarbajal Can you give me some more context on where you're getting this? I think the issue is one of not using a leading slash on your prefix?

I don't think reverting the relative routes as in this PR is a good idea, because then we break the fix made for the reverse proxy usage. (#164)

I can certainly see a case to be made that a leading slash should be checked for and included if absent in this location. Also, I spot that I missed line 19 of index-html.js, so that needs fixing too.

When I have a moment, I'll make a PR for it, or would you @simoncarbajal like to amend your PR to check for the slash and add it if missing?

@simoncarbajal
Copy link
Author

@jdhollander I just checked and adding a leading slash fixes the issue, thanks!
Checking for the leading slash and adding it if it's missing would be a nice time saver for the rest of us :)

@jdhollander
Copy link
Contributor

@simoncarbajal Glad you're working ok.

I can't see anything in the fastify docs that suggests a prefix without a leading slash is valid - all the examples have a leading slash. Therefore, I'm now not sure it's worth the effort to check for the slash.

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.

3 participants