Add cancellation token package #2
Conversation
jeskew
force-pushed
the
feature/cancellation-token
branch
from
f24e0df
to
a2b6999
Compare
jeskew
force-pushed
the
feature/cancellation-token
branch
from
a2b6999
to
7f7ed81
Compare
jeskew
force-pushed
the
feature/cancellation-token
branch
from
a5c1dc6
to
028ee7d
Compare
| * Creates a new Token object linked to this TokenSource (i.e., one that | ||
| * will signal cancellation when this source has been cancelled). | ||
| */ | ||
| get token(): Token { |
There was a problem hiding this comment.
Why use a getter here instead of a method? It seems a little weird here because the following is not intuitive:
const tokenSource = new TokenSource();
// expect next line to be 'true' but instead get 'false'
tokenSource.token === tokenSource.token;There was a problem hiding this comment.
Good point. I'll switch this to a method.
| /** | ||
| * Alias of this.cancellable | ||
| */ | ||
| get canBeCancelled(): boolean { |
There was a problem hiding this comment.
Why do we need both Token.cancellable and Token.canBeCancelled fields if they return the same thing?
There was a problem hiding this comment.
I thought canBeCancelled was more accurate but a bit verbose, but it's not necessary.
| /** | ||
| * Alias of this.canceled | ||
| */ | ||
| get isCancellationRequested(): boolean { |
There was a problem hiding this comment.
Same comment about using aliased fields.
| @@ -12,27 +12,20 @@ export class Token { | |||
| * Whether the associated operation may be cancelled at some point in the | |||
| * future. | |||
| */ | |||
| public readonly cancellable: boolean; | |||
| public readonly canBeCancelled: boolean; | |||
There was a problem hiding this comment.
Optional: I actually liked cancellable, but it doesn't really matter.
| @@ -50,6 +42,10 @@ export class TokenSource { | |||
| } | |||
| } | |||
|
|
|||
| getToken(): Token { | |||
There was a problem hiding this comment.
Minor: The docs for token should be moved here as well.
|
|
Add cancellation token package
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread. |
/cc @chrisradek