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

potential breaking change with baseDir-option #118

Open
Uzlopak opened this issue Jan 11, 2024 · 6 comments
Open

potential breaking change with baseDir-option #118

Uzlopak opened this issue Jan 11, 2024 · 6 comments

Comments

@Uzlopak
Copy link
Collaborator

Uzlopak commented Jan 11, 2024

I'm concerned this is a breaking change in behaviour and needs to be highlighted as such when it gets published - but would like a second opinion.

Prior to this change, if you set baseDir, the swagger static content would be served from the static folder included with this module. You could then set baseDir to point at another directory of content that should served in addition.

With this change, if you set baseDir it expects you to have copied the static folder included with this module into your baseDir.

So a working configuration prior to this PR will no longer work when this PR is applied.

Originally posted by @knolleary in #108 (comment)

@knolleary
Copy link
Contributor

@Uzlopak thanks - was in the middle of raising an issue as there were some related observations I wanted input on in relation to baseDir and this change.

PR #108 made some changes to how static content is served. There are two types of static content - the swagger-ui static content and then any app-specific static content that needs to be served alongside.

Before #108 was merged (and in the current latest release on npm, 2.0.1), the swagger-ui static content is always served from the static directory inside the npm module and gets served related to /documentation/static/.
baseDir could be used to point at an additional dir of static content that would get served related to /documentation.

With the changes in #108, the baseDir now also controls where the swagger-ui static content is served from. However that PR leads to the swagger-ui files being served from two places:

  • /documentation/static/index.html - as expected
  • /documentation/index.html - not expected

I picked index.html here, but the same is true for all of the files in swagger-ui/static such as swagger-ui.js. This because fastifyStatic is being registered twice for the same root value, but once with prefix set to static.

There are a few problems with this:

  1. A working configuration that has baseDir pointed at app-specific static content in 2.0.1 will be broken as it is now required for the swagger-ui content to have been copied into baseDir as well.
  2. The layout of the baseDir static files requires the swagger-ui files to be at the top level of that directory - not nested in a static folder
  3. The swagger-ui files get served from two places as described above.

There is an orthogonal issue that I believe is a security matter that I will disclose as per the security policy (if I can figure out all the right fields to enter).

@mcollina
Copy link
Member

What would you recommend? Would you like to send a PR?

@davidjbng
Copy link
Contributor

davidjbng commented Jan 11, 2024

We could have introduced an additional option e.g. serveDir and used that instead of baseDir to avoid breaking change.
I did forget to mention this idea in #108.

Is that an option here?

@knolleary
Copy link
Contributor

I think there are two separate concepts here that should be reflected in the configuration - as you suggest @davidjbng

  1. baseDir, as documented, should be the location of any additional static files that should be served relative to /documentation.
  2. uiStaticDir (naming is hard, but I think this balances clarity and consistency) is the location of the swagger-ui content and will be served relative to /documentation/static/

A 2.0.1 configuration will continue to work as before.

A user wanting to bundle the swagger-ui files into their own app will need to put them somewhere and set uiStaticDir

@davidjbng
Copy link
Contributor

davidjbng commented Jan 11, 2024

Agreed. I should have done it like that in the first place.
I can provide a PR tomorrow or you can as well, if you want to.
Thanks for clearing this up.

@mcollina
Copy link
Member

That'd be amazing thx!

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

No branches or pull requests

4 participants