fastify-swagger-ui icon indicating copy to clipboard operation
fastify-swagger-ui copied to clipboard

Add option for `noRedirect` to serve UI directly at the route prefix

Open jdhollander opened this issue 9 months ago • 3 comments

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.

Checklist

noRedirect option

I have introduced a new option, which will disable the redirect to /static/index.html for the documentation UI. On visiting the documentation route, the UI is returned without redirection, and the url stays clean.

await fastify.register(require('@fastify/swagger-ui'), {
  routePrefix: '/documentation',
  noRedirect: true,
})

jdhollander avatar May 01 '24 14:05 jdhollander

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?

jdhollander avatar May 02 '24 09:05 jdhollander

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 https://github.com/fastify/fastify-swagger/pull/39 https://github.com/fastify/fastify-swagger/pull/56 https://github.com/fastify/fastify-swagger/issues/65

Uzlopak avatar May 02 '24 10:05 Uzlopak

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.

mcollina avatar May 02 '24 14:05 mcollina

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?

jdhollander avatar Jun 10 '24 10:06 jdhollander

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?

mcollina avatar Jun 11 '24 10:06 mcollina

Can you update the OP?

mcollina avatar Jun 11 '24 13:06 mcollina

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 avatar Jun 11 '24 13:06 jdhollander

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.

jdhollander avatar Jun 11 '24 13:06 jdhollander

@mcollina

Yes, dropping it is imho the best option.

Uzlopak avatar Jun 13 '24 08:06 Uzlopak