-
Notifications
You must be signed in to change notification settings - Fork 113
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
Improve types, doc, allow spec on ApiSchema, fix showExample #339
Improve types, doc, allow spec on ApiSchema, fix showExample #339
Conversation
|
Someone is attempting to deploy a commit to a Personal Account owned by @rohit-gohri on Vercel. @rohit-gohri first needs to authorize it. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
I like the idea, correct me if I'm wrong but you want to avoid having to add the specs into the configuration and instead want to treat the spec as a doc? |
Yes, I cannot use the global configuration of redocusaurus as my specs are versionned. On every version of our software, we have the previous and the new spec inside the doc |
Makes sense, what do you think about the idea of adding similar version commands as docs plugin and auto loading all yamls from an openapi folder with the name as ids? There is some amount of preprocessing that needs to be done on the specs, so directly importing might work for you but not others |
When you speak about preprocessing spec files, you think about several spec files to merge ? I don't know every use case available. Managing versionned spec files + preprocessing can be quite hard to do, like how do you know which spec file you need to use with the provided ID ? You will have, for one ID, several specs files, one for each version of the website. In my project, we create some new microservice sometimes, so spec file can be unavailable for older version (or newer version if we remove a microservice) |
export type RedocProps = SpecProps & { | ||
/** | ||
* FIXME - incorrect name - should be isExternalUrl | ||
*/ | ||
isSpecFile?: boolean; | ||
className?: string; | ||
optionsOverrides?: RedocRawOptions; | ||
/** | ||
* External URL to load spec file from | ||
* FIXME - incorrect name - should be externalUrl | ||
*/ | ||
url?: string; | ||
}; |
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.
need your review. I think isSpecFile
can be removed now since I've added a tester that check if the user provided an url starting with http
Other point: When you provide url to Redoc, it means you want to render an external swagger link. It should be renamed... (or at least documented)
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.
I think isSpecFile can be removed now since I've added a tester that check if the user provided an url starting with http
It could also be a relative url to a yaml file in the static directory.
When you provide url to Redoc, it means you want to render an external swagger link. It should be renamed... (or at least documented)
Currently it works as the download url as the processed one removes some formatting which makes it hard to read
The external url is not intended, will see if the 2 usecases can be separated
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.
Feel free to commit over my PR or tell me what to do.
This is weak part of this MR
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.
It could also be a relative url to a yaml file in the static directory.
Yes, I've made a function that computes if the file is inside the doc or on another website. That's just checking if the URL start with http, no need to ask something as a config attribute
I was thinking the same way markdown files are versioned, load files and auto generate ids for them based on path. But I think your approach should also be supported, it makes sense since we are exposing components that can be used in Mdx. I'll create a separate issue for versioning ( EDIT: created - #341 ), I have an idea, will tag you when a PR is ready and you can see if it would work for you |
@serut Depending on how much time you have, you can try to make it backward compatible; or can also just let it be, and in that case I'll cherry pick the relevant changes in a separate PR and add you as a co-author. Thanks for opening the PR, I really like the idea of being able to just import openapi docs in Mdx without any configuration |
acd3732
to
438de62
Compare
So far so great, everything looks backward compatible, and types are easier to maintain. I let you review this MR |
Did you had time to review this merge request @rohit-gohri ? |
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.
Hey Sorry for the late review, but there are still a few breaking changes and some props that don't work everywhere. If you are okay with it, I can merge it but I'll have to make some changes before releasing? What do you think?
@@ -33,6 +33,10 @@ const version = require('../package.json').version; | |||
|
|||
export { PluginOptions, PluginDirectUsageOptions, loadRedoclyConfig }; | |||
|
|||
function getIsExternalUrl(url = '') { |
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.
this still doesn't handle relative urls
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.
Do you really want to handle relative URL (inside the doc) as external API ?
I don't get the usecase.
Either you use RedocStandalone if the schema is outside of the doc, either you use the real Redoc and it handles it by loading 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.
It supports it currently, so gotta keep 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.
Oh... I didn't think it was intented.
It's more a miss-usage of the API
|
||
if ((isDevMode && isSpecFile === false) || !spec) { | ||
if (getIsExternalUrl(url)) { |
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.
This is not what we want, external urls should still be parsed at build time for server side rendering
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.
Ok, maybe I miss the usecase... What is the difference between running in client mode and server side rendering mode for props in this component ?
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.
The difference is if the html is generated at build time (and hence works for SEO and works without javascript) or on client side
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.
Yes but in terms of props, what's the difference ?
lightThemeOptions: import('redoc/typings').RedocRawOptions; | ||
} | ||
|
||
type RedocProps = SpecProps & { |
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.
this is what I was talking about here - https://github.com/rohit-gohri/redocusaurus/pull/339/files#r1518118853
The id is now available as a prop in RedocProps also though SpecProps. But it doesn't work
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.
Hmm strange, when I run the website it looks ok.
But I can be wrong as every OpenAPI example in the website contains approximative content. We should make many OpenAPI example and replace Dog, Cat ... with new animals (Elephant...) to be sure the right OpenAPI is loaded
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.
It looks okay, but this compnent now has props which don't do anything
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.
Are you sure it's not working with id
? Everything is given to useSpec
so it should works
Should be added there in that case https://github.com/rohit-gohri/redocusaurus/pull/339/files#diff-28d0aac1a088788b029652a42c7bfb38079857218f6f9f29be02cf9dff768a2cR43
You can merge it on another branch than master to do some work on it easier (if you cannot push commit on my fork). Or merge it on master, as you want |
162cdc6
into
rohit-gohri:feature/pass-spec-directly
I wanted to add the ability to pass the
spec
param to ApiSchema, I end up with so many changes !!There is still some FIXME on the code, I let you review and tell me what you think about it