-
Notifications
You must be signed in to change notification settings - Fork 32
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
Feature: Document server - redoc #38
Conversation
Codecov ReportBase: 91.86% // Head: 92.08% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #38 +/- ##
===========================================
+ Coverage 91.86% 92.08% +0.21%
===========================================
Files 208 215 +7
Lines 2901 2980 +79
Branches 345 346 +1
===========================================
+ Hits 2665 2744 +79
Misses 172 172
Partials 64 64
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
6d4802a
to
3716047
Compare
3716047
to
8efee48
Compare
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.
Beside some some suggestion, others LGTM 👍
import { TYPES } from '@vulcan-sql/serve/containers'; | ||
import * as compose from 'koa-compose'; | ||
|
||
@VulcanInternalExtension() |
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.
How about to setup the all different document server type under one options module name, because seems different document server extensions both use the same DocumentOptions
.
like below:
api-doc:
spec: [oas3]
server: [redoc, ...]
And your document server middleware read the doc
options, and according to your server
to create each document type server ?
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.
Did you mean to change to property name from document
to api-doc
and turn it from internal options to module configuration?
I think it will be great for renaming, but because it a cross-package config, that is, spec for both build
and serve
package and server for serve
package, I'd prefer to use internal options to do some validations.
import * as path from 'path'; | ||
import { inject } from 'inversify'; | ||
|
||
@VulcanInternalExtension('redoc') |
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.
Follow by above documentServerMiddleware
suggestion. Here the @VulcanInternalExtension('redoc')
may could change to documentServerMiddleware
internal extension module name ?
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 makes sense to merge the options of redoc with 'api-doc' options because it is a built-in router, but I'll prefer using internal options instead of module configuration.
/** Target specification of our APIs, e.g. OpenAPI, Tinyspec ...etc. */ | ||
specs?: (string | DocumentSpec)[]; | ||
folderPath?: string; | ||
server?: (string | DocumentServerType)[]; |
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.
Maybe we could rename the propety server
and DocumentServerType
e.g: from server
To the renderType
, and the DocumentRenderType
or DocumentRouterType
DocumentType
?
Because seems that each document server extension still uses the same port in vulcan server with a different router.
If we use the server
, it looks like each document server type use a different port to launch.
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.
Similar suggestion for documentServerMiddleware
and documentServer
extension, we could change the server
to another word, e.g: render, router... and so on, to prevent each document server will create a new server with a different port
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.
Thanks, I think router will be better, I'll update all of them.
move api base url to /api for incoming pages like /doc /catalog ...etc.
… from build to core We're going to implement document server at serve package, we'll need to share document options almong core, build, and serve packages. So I move these options to core package.
5d94fe9
to
7f31291
Compare
@kokokuo I've solve the conflicts and issues, thanks a lot. |
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.
LGTM 👍 Thanks ~
Description
Host redoc on
/doc
path (configurable) and provide restful API spec.Note
/api
prefix, e.g./user
->/api/user
.