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

Serve UI directly at the route prefix, rather than redirecting #144

Merged
merged 1 commit into from
Jun 13, 2024

Conversation

jdhollander
Copy link
Contributor

@jdhollander jdhollander commented May 1, 2024

This PR is related to #117. I like the suggestion, and it would be beneficial to me to have this feature, so I've implemented it.

Previously, when visiting the swagger UI route (default /documentation), the page redirected to /documentation/static/index.html to display the UI. This no longer is the case, rather the UI is served directly.

This should not affect any existing links to /documentation/static/index.html as they will be redirected to /documentation and function as before.

Checklist

.prettierignore Outdated Show resolved Hide resolved
Copy link
Collaborator

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

noRedirect is not about not redirecting but actually about some routing option. This seems kind of wrong.

@jdhollander jdhollander force-pushed the noRedirect-option branch from 5328b13 to 09010c2 Compare May 2, 2024 09:05
@jdhollander
Copy link
Contributor Author

noRedirect is not about not redirecting but actually about some routing option. This seems kind of wrong.

Thanks for the feedback. Any suggestion on what I should call it? serveIndexAtRootPrefix? noStaticIndexRedirect?

Otherwise, is the general approach to this ok?

@Uzlopak
Copy link
Collaborator

Uzlopak commented May 2, 2024

Why do we even redirect? I have unfortunately currently no motivation to deep dive into the topic. But maybe not redirecting but directly serving the expected files should be the desired result?

Maybe related to
fastify/fastify-swagger#39
fastify/fastify-swagger#56
fastify/fastify-swagger#65

@mcollina
Copy link
Member

mcollina commented May 2, 2024

I did the redirect because I was too lazy to figure out how to make that rendering work correctly. At the beginning, the HTML was entirely static, so just redirecting it makes sense.

Maybe we should flip it to true by default.

@jdhollander
Copy link
Contributor Author

I did the redirect because I was too lazy to figure out how to make that rendering work correctly. At the beginning, the HTML was entirely static, so just redirecting it makes sense.

Maybe we should flip it to true by default.

Any decision on this?

@mcollina
Copy link
Member

I think dropping the redirect entirely seems the best option. Would you like to take care of that?

@Uzlopak would you be ok if that was the case?

@jdhollander jdhollander force-pushed the noRedirect-option branch 2 times, most recently from 0c494c8 to 27a8126 Compare June 11, 2024 12:12
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

@mcollina
Copy link
Member

Can you update the OP?

@mcollina mcollina requested a review from Uzlopak June 11, 2024 13:24
@jdhollander
Copy link
Contributor Author

Can you update the OP?

There's a couple of changes to make - issues with trailing slashes that I am ironing out and test failures. Almost there, and I'll rebase it on updated master.

@jdhollander
Copy link
Contributor Author

Ok, code is updated and I think in a good state. The changes are mainly because you can access the documentation route with or without a trailing slash, and this changes the behaviour of the relative imports used in the html. So, I have needed to handle this, by pasing the url of the request into the html template. If there's a simpler way, I'm all ears, but I wanted to make sure this honoured either trailing slash option (which I believe can be overidden at the server level, so this should handle both just in case?)

Tests are updated to reflect this, and new e2e test cases make sure that the import of static assets, and the correct url for the json spec is used (which it wasn't before, but I hadn't noticed). I hope that all makes sense.

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

Can you update the OP?

@jdhollander jdhollander changed the title Add option for noRedirect to serve UI directly at the route prefix Serve UI directly at the route prefix, rather than redirecting Jun 11, 2024
@Uzlopak
Copy link
Collaborator

Uzlopak commented Jun 13, 2024

@mcollina

Yes, dropping it is imho the best option.

@mcollina mcollina merged commit 7260c23 into fastify:master Jun 13, 2024
18 of 21 checks passed
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.

4 participants