Skip to content
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

fix: issue where custom assets are not injected in html #2487

Merged
merged 1 commit into from
Jun 19, 2023
Merged

fix: issue where custom assets are not injected in html #2487

merged 1 commit into from
Jun 19, 2023

Conversation

joshunrau
Copy link
Contributor

@joshunrau joshunrau commented Jun 17, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Options passed to SwaggerModule.setup are ignored. The customOptions object passed to buildSwaggerHTML is malformatted. Custom parameters that should be contained within the base of customOptions are actually contained within swaggerOptions, which itself contains another object swaggerOptions. For example, this is the object passed to buildSwaggerHTML when you run npm run start:dev:

{
  jsonDocumentUrl: '/api-docs-json',
  yamlDocumentUrl: '/api-docs-yaml',
  swaggerOptions: {
    customSiteTitle: 'Demo API - Swagger UI 1',
    swaggerOptions: {
      persistAuthorization: true,
      defaultModelsExpandDepth: -1,
      syntaxHighlight: [Object],
      tryItOutEnabled: true
    },
    customfavIcon: '/public/favicon.ico',
    customCssUrl: '/public/theme.css'
  }
}

I changed how the options are parsed from the customOptions object in buildSwaggerHTML.

Issue Number: #2481

What is the new behavior?

I implemented a quick fix to resolve the bug. Now, extracted the options from a merger of customOptions and customOptions.swaggerOptions. This is not an ideal solution, but it seems this object structure is assumed in several other places in the code base, and applying a more systematic fix was non-trivial, as someone who is unfamiliar with the project.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

I added tests for all of the custom assets so this will not inadvertently break again in the future.

@kamilmysliwiec
Copy link
Member

Thanks for your contribution! I took a slightly different approach as it seems that the regression was introduced in this PR #2392 where we pass the outer options object instead of the Swagger-specific custom options object down to the function you modified. Still, I'm gonna merge this PR as it adds some valuable e2e tests and will prevent us from introducing similar breaking changes in the future!

@kamilmysliwiec kamilmysliwiec merged commit fb22900 into nestjs:master Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants