-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
fix(typescript-angular): enable "exactOptionalPropertyTypes" and ensure assignments align with type definition #20451
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
Changes from 1 commit
70943ad
1ac289b
dacf239
16c722c
6f7b9b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,26 +66,30 @@ export class {{configurationClassName}} { | |
| */ | ||
| credentials: {[ key: string ]: string | (() => string | undefined)}; | ||
|
|
||
| constructor(configurationParameters: {{configurationParametersInterfaceName}} = {}) { | ||
| this.apiKeys = configurationParameters.apiKeys; | ||
| this.username = configurationParameters.username; | ||
| this.password = configurationParameters.password; | ||
| this.accessToken = configurationParameters.accessToken; | ||
| this.basePath = configurationParameters.basePath; | ||
| this.withCredentials = configurationParameters.withCredentials; | ||
| this.encoder = configurationParameters.encoder; | ||
| if (configurationParameters.encodeParam) { | ||
| this.encodeParam = configurationParameters.encodeParam; | ||
| constructor({ accessToken, apiKeys, basePath, credentials, encodeParam, encoder, password, username, withCredentials }: {{configurationParametersInterfaceName}} = {}) { | ||
| if (apiKeys) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are all guarded now, but beforehand they weren't, if the containing object was truthy all of them would have been set, no matter if truthy or not. Do we need to look into it in more detail on whether that behavior changes are fully backwards compatible?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is absolutely correct. I have adapted it by explicitly checking for |
||
| this.apiKeys = apiKeys; | ||
| } | ||
| else { | ||
| this.encodeParam = param => this.defaultEncodeParam(param); | ||
| if (username) { | ||
| this.username = username; | ||
| } | ||
| if (configurationParameters.credentials) { | ||
| this.credentials = configurationParameters.credentials; | ||
| if (password) { | ||
| this.password = password; | ||
| } | ||
| else { | ||
| this.credentials = {}; | ||
| if (accessToken) { | ||
| this.accessToken = accessToken; | ||
| } | ||
| if (basePath) { | ||
| this.basePath = basePath; | ||
| } | ||
| if (withCredentials) { | ||
| this.withCredentials = withCredentials; | ||
| } | ||
| if (encoder) { | ||
| this.encoder = encoder; | ||
| } | ||
| this.encodeParam = encodeParam ?? (param => this.defaultEncodeParam(param)); | ||
| this.credentials = credentials ?? {}; | ||
| {{#authMethods}} | ||
|
|
||
| // init default {{name}} credential | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use
??Or||?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I misunderstand the suggestion to use the nullish coalescing operator, but if I use it I would have to specify a default value for
withCredentials.withCredentialsis type ofbooleanand is optional, see here. WithexactOptionalPropertyTypesan assignment ofundefinedwouldn't be allowed.withCredentials: withCredentials ?? falseseemed more like a breaking changeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I would be fine to go this route. I had a rough look at the Angular code and it seems that no distinction is made between
undefinedandfalse.