feat: allow to disable Swagger UI #2840
Conversation
|
|
||
| httpAdapter.get( | ||
| normalizeRelPath(`${finalPath}/swagger-ui-init.js`), | ||
| (req, res) => { | ||
| res.type('application/javascript'); | ||
| const document = getBuiltDocument(); |
There was a problem hiding this comment.
getBuiltDocument handles internally a kind of "cache" of the document, to avoid rebuilding it for each request
| if (!document) { | ||
| document = lazyBuildDocument(); | ||
| if (!swaggerUiHtml) { | ||
| swaggerUiHtml = buildSwaggerHTML(baseUrlForSwaggerUI, swaggerOptions); | ||
| } | ||
|
|
||
| if (options.swaggerOptions.patchDocumentOnRequest) { | ||
| const documentToSerialize = | ||
| options.swaggerOptions.patchDocumentOnRequest(req, res, document); | ||
| const htmlPerRequest = buildSwaggerHTML( | ||
| baseUrlForSwaggerUI, | ||
| documentToSerialize, | ||
| options.swaggerOptions | ||
| ); | ||
| return res.send(htmlPerRequest); | ||
| } | ||
|
|
||
| if (!html) { | ||
| html = buildSwaggerHTML( | ||
| baseUrlForSwaggerUI, | ||
| document, | ||
| options.swaggerOptions | ||
| ); | ||
| } |
There was a problem hiding this comment.
buildSwaggerHTML was not using the document internally, so I simplified this part
| if (!document) { | ||
| document = lazyBuildDocument(); | ||
| } | ||
|
|
||
| if (options.swaggerOptions.patchDocumentOnRequest) { | ||
| const documentToSerialize = | ||
| options.swaggerOptions.patchDocumentOnRequest(req, res, document); | ||
| const htmlPerRequest = buildSwaggerHTML( | ||
| baseUrlForSwaggerUI, | ||
| documentToSerialize, | ||
| options.swaggerOptions | ||
| ); | ||
| return res.send(htmlPerRequest); | ||
| if (!swaggerUiHtml) { | ||
| swaggerUiHtml = buildSwaggerHTML(baseUrlForSwaggerUI, swaggerOptions); | ||
| } | ||
|
|
||
| if (!html) { | ||
| html = buildSwaggerHTML( | ||
| baseUrlForSwaggerUI, | ||
| document, | ||
| options.swaggerOptions | ||
| ); | ||
| } |
There was a problem hiding this comment.
buildSwaggerHTML was not using the document internally, so I simplified this part
| @@ -66,7 +66,6 @@ function toTags( | |||
| */ | |||
| export function buildSwaggerHTML( | |||
| baseUrl: string, | |||
| swaggerDoc: OpenAPIObject, | |||
There was a problem hiding this comment.
This was dead code
| it.each([ | ||
| '/apidoc', | ||
| '/apidoc/', | ||
| '/apidoc/swagger-ui-bundle.js', | ||
| '/apidoc/swagger-ui-init.js' | ||
| ])('should not serve "%s"', async (file) => { | ||
| const response = await request(app.getHttpServer()).get(file); | ||
|
|
||
| expect(response.status).toEqual(404); | ||
| }); |
There was a problem hiding this comment.
All Swagger UI resources are throwing 404 when disabled
lucas-gregoire
changed the title
|
@kamilmysliwiec Would it be possible to review and/or merge this PR please ? |
There was a problem hiding this comment.
LGTM; not tested.
Formatting-wise, I'd ask to undo gratuitous changes (removing trailing comma, quotes, semicolons, line-splitting, etc) to minimize diff, preserve git blame, and simplify review — IF I was maintainer of this code, who I'm not.
I've landed here at all, because was investigating a breakage of Swagger-UI in a Nest app, caused by switching to a server-side bundler (esbuild), caused by other reasons.
| /** | ||
| * If `false`, only API definitions (JSON and YAML) will be served (on `/{path}-json` and `/{path}-yaml`). | ||
| * This is particularly useful if you are already hosting a Swagger UI somewhere else and just want to serve API definitions. | ||
| * Default: `true`. | ||
| */ | ||
| swaggerUiEnabled?: boolean; |
There was a problem hiding this comment.
+1 for aligning the code with what's actually documented at https://docs.nestjs.com/openapi/introduction#setup-options
There was a problem hiding this comment.
Why is it in the docs but not in the code? Made me scratch my head for half an hour..
There was a problem hiding this comment.
agreeing to @motabass, the docs are live already, can this be merged please?
There was a problem hiding this comment.
Indeed, would be nice if this would be merged, it's quite confusing that it's in the docs but you can't actually use it 😬
There was a problem hiding this comment.
i agree merge it, the docs are live already, i want use selfdefine openapi-ui
There was a problem hiding this comment.
I'm sorry that both PRs haven't been merged simultaneously (or at least the code before the docs). The PR for the docs was merged in 1 day, while this one has been waiting (and ready) for 4 months 😿
There was a problem hiding this comment.
For those who want to try this feature before its release, I've published a forked package @lsge/swagger that you can install as a replacement for @nestjs/swagger:
Change this in your package.json:
- "@nestjs/swagger": "7.4.0",
+ "@nestjs/swagger": "npm:@lsge/[email protected]",
|
i support merge it, the docs are live already, i want use selfdefine openapi-ui |
|
Yes, came here because the docs already mention the flag swaggerUiEnabled and I was confused that it didn't work until I saw this PR... |
|
@kamilmysliwiec can you merge? |
|
lgtm |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: none
Currently when using
SwaggerModule.setupit serves all files of Swagger UI and API definitions generated on the fly (JSON and YAML). It isn't possible for now to only serve API definitions.I see two uses-cases for this feature:
What is the new behavior?
This PR introduces a way to disable the Swagger UI to only generate API definitions.
A new flag
swaggerUiEnabledis available inSwaggerModule.setupoptions.Does this PR introduce a breaking change?
Other information
In this PR, I also improved some documentation and removed dead code