Skip to content

Conversation

@OsamaSBCrea
Copy link
Contributor

@OsamaSBCrea OsamaSBCrea commented Mar 6, 2023

PR Description

Need to add abort capability for async operations in the SDK to be able to cancel long operations like pollTransaction from the console (See here). The changes adds the abort capability for pollTransaction API, by passing an AbortSignal object reference that can be triggered from outside.

@OsamaSBCrea OsamaSBCrea requested a review from denisgursky March 6, 2023 12:08
constructor(message?: string) {
super(message);

this.name = 'AbortError';
Copy link
Contributor

@meruyert93 meruyert93 Mar 6, 2023

Choose a reason for hiding this comment

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

sorry for jumping here, just noticed it.
Why did you decide to extend it and put name? Is there any reason for that?
since as I know error name would be already 'AbortError' when it is aborted. Therefore, I think it doesn't have to be extended.

Here it is from documentation:
https://developer.mozilla.org/en-US/docs/Web/API/AbortController/abort

The reason why the operation was aborted, which can be any JavaScript value. If not specified, the reason is set to "AbortError"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main reason is that the error thrown through browser's fetch API is of type DOMException which is different than the one through the node-fetch API, which is explicitly defined as AbortError. Another thing, in the changes here, the abort is not passed to the fetch function, it's only used to break the polling loop and reject the promise manually.
It might be removed if we decided to pass the abort signal to the fetch API, but that requires further changes that should be discussed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, I opened the PR to establish a detailed discussion about the usage of abort signal in the SDK. Furthermore, I noticed that other SDKs define their own types of AbortController and AbortSignal which might be needed somewhere (AWS, Azure)

@OsamaSBCrea OsamaSBCrea changed the title RAI-10177 add abort signal to poll transaction api RAI-10177 add abort signal to get apis Mar 8, 2023
@OsamaSBCrea OsamaSBCrea requested a review from larf311 March 9, 2023 16:17
@OsamaSBCrea OsamaSBCrea changed the title RAI-10177 add abort signal to get apis RAI-10177 add abort signal to all apis Mar 14, 2023
@OsamaSBCrea OsamaSBCrea changed the title RAI-10177 add abort signal to all apis RAI-10177 add abort signal pollTransaction May 17, 2023
}
}

export class AbortError extends Error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please export it in the entry point.

@denisgursky denisgursky self-requested a review May 17, 2023 18:52
@OsamaSBCrea OsamaSBCrea merged commit 7c730eb into main May 18, 2023
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.

3 participants