Skip to content

Conversation

@xTCry
Copy link

@xTCry xTCry commented May 1, 2022

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?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@xTCry xTCry changed the title chore(types): optional type in options of add security refactor(types): improved security scheme May 1, 2022
Comment on lines -30 to -31
.addApiKey({ type: 'apiKey' }, 'key1')
.addApiKey({ type: 'apiKey' }, 'key2')
Copy link
Member

Choose a reason for hiding this comment

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

This would make it a breaking change, but would it make more sense to bring the name to be the first parameter instead of the options?

Copy link
Author

Choose a reason for hiding this comment

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

@jmcdo29 , yep, you're right. Fixed

Comment on lines +141 to +142
options: Partial<Omit<HttpSchemeObject, 'type'>> = {},
name = 'basic'
Copy link
Member

Choose a reason for hiding this comment

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

Making the change I mentioned above here so it would be

Suggested change
options: Partial<Omit<HttpSchemeObject, 'type'>> = {},
name = 'basic'
name = 'basic',
options: Partial<Omit<HttpSchemeObject, 'type'>> = {}

Copy link
Author

Choose a reason for hiding this comment

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

@jmcdo29, Yes, this can also be added, but is it better in a minor update?..

@xTCry
Copy link
Author

xTCry commented May 1, 2022

@jmcdo29 Yes, I agree. But this is likely to be a breaking change

@micalevisk
Copy link
Member

@xTCry I guess we could keep both and mark the old one as deprecated

@kamilmysliwiec
Copy link
Member

What's the reason for this change?

@xTCry
Copy link
Author

xTCry commented Sep 1, 2022

I apologize for the long answer.

What's the reason for this change?
@kamilmysliwiec

This is a typing improvement, based on the official Swagger documentation. It is also simplified to access some methods without specifying unnecessary parameters.

For example, why write .addApiKey({ type: 'apiKey'}, 'key1'), if the name of the method speaks for itself that it accepts only the type apiKey.
Also, HttpSchemaObject accepts bearerFormat, OpenIdConnectSchemeObject can use openIdConnectUrl. But they are not used in other ways

@car1ot

This comment was marked as spam.

@nestjs nestjs locked and limited conversation to collaborators Oct 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants