-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[core-util] Edit delay() #24801
[core-util] Edit delay() #24801
Conversation
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.
Looks good. Thanks for the improvement!
API change check API changes are not detected in this pull request. |
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.
I love when the red subtraction number is bigger than the green addition number.
sdk/core/core-util/src/delay.ts
Outdated
@@ -28,38 +27,24 @@ export interface DelayOptions { | |||
*/ | |||
export function delay(timeInMs: number, options?: DelayOptions): Promise<void> { | |||
return new Promise((resolve, reject) => { | |||
let timer: ReturnType<typeof setTimeout> | undefined = undefined; | |||
let onAborted: (() => void) | undefined = undefined; | |||
|
|||
const rejectOnAbort = (): void => { |
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.
since we're cleaning this up, it might be nicer to have these simply be named functions instead of anonymous ones bound to a local const:
function rejectOnAbort() {
|
||
const StandardAbortMessage = "The operation was aborted."; | ||
const StandardAbortMessage = "The delay was aborted."; |
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.
I think some Storage tests at least might be sniffing the message, so you could see some failures with brittle tests. I tried to clean most of that up recently, but fair warning. :)
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.
The packages in the CI pipeline seem to be happy but if you come across any such failure, please pass it my way and I will address it.
Minor edit to use modern syntax and simpler control flow.