-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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
perf(tooling): Upgrade to TypeScript 4.8 #4207
Conversation
@@ -23,9 +23,9 @@ export class PushoverApi implements ICredentialType { | |||
requestOptions: IHttpRequestOptions, | |||
): Promise<IHttpRequestOptions> { | |||
if (requestOptions.method === 'GET') { | |||
Object.assign(requestOptions.qs, { token: credentials.apiKey }); | |||
Object.assign(requestOptions.qs ?? {}, { token: credentials.apiKey }); |
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.
we really need to revisit our usage of Object.assign
. it's seems to be used unnecessarily and incorrectly in many places. This should either be
requestOptions.qs = Object.assign(requestOptions.qs ?? {}, { token: credentials.apiKey })
or
requestOptions.qs = requestOptions.qs ?? {}
Object.assign(requestOptions.qs, { token: credentials.apiKey })
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.
Moved to use a nullish check instead of ?? {}
, to remain closer to current behavior. Object.assign(undefined, obj)
throws so we know the target is defined in these calls. Dug deeper and it looks like root cause is Object.assign
overloads now constraining the target param to be a subset of {}
.
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.
LGTM
merged master back into make sure no newly added code was gonna break after we merge this.
Once CI is green, let's merge.
Got released with |
* ⬆️ Upgrade to TypeScript 4.8 * 🔥 Remove unneeded setting * 📦 Update `package-lock.json` * ⏪ Restore `skipLibCheck` * 📦 Re-update `package-lock.json` * ♻️ Apply feedback * ♻️ Add check to new WhatsApp node * 📦 Update `package-lock.json` * Update package-lock.json * ran `npm run lintfix` Co-authored-by: कारतोफ्फेलस्क्रिप्ट™ <aditya@netroy.in>
No description provided.