-
-
Notifications
You must be signed in to change notification settings - Fork 233
docs should be available on a subpath of / #9
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
Conversation
danielgtaylor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments, nice work! 👍
I also think this middleware needs to be updated:
https://github.com/danielgtaylor/huma/blob/master/middleware.go#L295-L316
docs.go
Outdated
| // RapiDocTemplate is the template used to generate the RapiDoc. It needs two args to render: | ||
| // 1. the title | ||
| // 2. the path to the openapi.yaml file | ||
| var RapiDocTemplate = `<!doctype html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be mutable? If not, we may want a func to return the string. Could also just be a func that renders the template, so you can take in args to describe the inputs instead of using comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, It'd be better to just export a function that would take args, instead of forcing a consumer of the module to know about the internals of a template.
I initially took the "export templates" approach because it helped me minimize the changes in all of the docs-Handlers in docs.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
functionized this: 2852753
options.go
Outdated
| // | ||
| // DEPRECATED! Use `DocsDomType` instead! | ||
| func DocsHandler(f Handler) RouterOption { | ||
| fmt.Println("This option is deprecated, use `DocsDomType` instead") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be supported without deprecating it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strictly speaking, yes. Where it gets weird is if you use the DocsRoutePrefix RouterOption to change the prefix of the where the docs live.
E.g.
NewServer("api", "version", DocsRoutePrefix("/prefix"), DocsHandler(ReDocHandler(title))))
Since DocsHandler is just setting a function to call when /docs is hit, and ReDocHandler isn't aware of the new prefix, it felt weird to have both DocsHandlers and DocsRoutePrefixes.
A way to get around that would be to call:
NewServer("api", "version", DocsRoutePrefix("/prefix"), DocsDomType("redoc")))
which would give the user the ability to use ReDoc, while also ensuring that the docs are reachable at /prefix/docs.
Which is why I went for the deprecation route.
options.go
Outdated
| } | ||
|
|
||
| // DocsDomType sets the presentation for the docs UI. Valid values are: | ||
| // "rapi" (default), "redoc", or "swagger" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constants or a custom type for the inputs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They should be constants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
constantized stuff at 1c74420
@danielgtaylor I updated the addservicelinkmiddleware at: e1babc9. there's some stuff that could be DRY'd up, but that would come at the cost of changing the interface of the methods involved in the commit. I learned toward not changing the interface, but understand that it'd probably be better to modify the interface (at the risk of breaking anything that depends on that method) |
Previously, the
/docsendpoint was static, meaning that if my huma-powered app was meant to be accessed from/myapp/*the docs could not be accessed.This PR addresses that (in a pretty hacky way) by:
This work should invalidate #8 .