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

Initial Implementation #1

Merged
merged 13 commits into from
Oct 1, 2022
Merged

Initial Implementation #1

merged 13 commits into from
Oct 1, 2022

Conversation

Uzlopak
Copy link
Collaborator

@Uzlopak Uzlopak commented Sep 5, 2022

This is the first implementation of the swagger-ui plugin.

Only options which effect the exposeRoute and the swagger-ui from @fastify/swagger should be covered by this plugin. exposeRoute as an option was removed, as it is implied that we want to expose the routes if we use this plugin.

Let us talk about if the initial options and separation is fine.

Still have to fix some tests, but it should already be self explainatory.

So please let us discuss if it is meeting our needs.

Checklist

lib/routes.js Outdated Show resolved Hide resolved
test/csp.test.js Outdated Show resolved Hide resolved
Co-authored-by: Frazer Smith <frazer.dev@outlook.com>
Copy link
Member

@Fdawgs Fdawgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can get rid of .npmignore if it is an exact copy of .gitignore, as NPM will fall back to using .gitignore if no .npmignore exists. See https://docs.npmjs.com/cli/v8/using-npm/developers#keeping-files-out-of-your-package

@Uzlopak
Copy link
Collaborator Author

Uzlopak commented Sep 30, 2022

removed the .npmignore

I still have to reactivate some unit tests and then it should be good to go.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina merged commit 7357acb into master Oct 1, 2022
@Uzlopak Uzlopak deleted the initial branch October 1, 2022 09:20
@Uzlopak Uzlopak mentioned this pull request Oct 5, 2022
4 tasks
@fmenis
Copy link

fmenis commented Apr 1, 2023

@Uzlopak I don't really agree with exposeRoute as an option was removed, as it is implied that we want to expose the routes if we use this plugin.
Imho, the exposeRoute configuration is very useful.

So, from version 8, if we don't want to expose the routes documentation in production, we must not register the plugin at all?
Before version 8, it was very easy exposeRoute: process.env.NODE_ENV !== ENV.PRODUCTION

@Eomm
Copy link
Member

Eomm commented Apr 2, 2023

Note that you need to write:

if(process.env.NODE_ENV !== ENV.PRODUCTION){
  fastify.register(swaggerUi)
}

What would be a better option?

@fmenis
Copy link

fmenis commented Apr 2, 2023

@Eomm for me, the best option would be the one exposed by the exposeRoute flag.
I find it more elegant than dynamically registering the plugin.
But it doesn't matter, as you (and I in the first post) indicated, just register the plugin dynamically and all works fine.

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.

5 participants