Skip to content
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

fix(types): change the type guard on isCancel #5595

Open
wants to merge 5 commits into
base: v1.x
Choose a base branch
from

Conversation

adimit
Copy link

@adimit adimit commented Mar 10, 2023

This fixes #5153.

Change the type guard on isCancel to CanceledError<any>. This makes more sense as it reflects what isCancel is actually doing. In fact, if I'm not mistaken, the Cancel type is no longer in the project. It got removed in 7f12366. It should probably also be removed from the types, but I didn't do it in this PR, as I wanted to keep the change minimal.

I needed to declare name on CanceledError because the types CanceledError and AxiosError need to be structurally different from each other in order for the type guard on isCancel to work properly. From the point of view of TypeScript, declaring CanceledError without introducing some new structure is a mistake, as both types are essentially aliases now. (TS is a structural typing system.) I did not opt to make __CANCEL__: boolean part of CanceledError's structure, as that seemed counterintuitive. I don't think users of this library should necessarily know it exists or rely on its existence.

adimit and others added 2 commits March 10, 2023 21:39
… to `CanceledError<any>`. This makes more sense as it reflects what
`isCancel` is actually doing. In fact, if I'm not mistaken, the
`Cancel` type is no longer in the project. It got removed in
7f12366. It should probably also be removed from the types.
@g1eny0ung
Copy link

Encountered this problem too. @DigitalBrainJS Mind taking a look at this PR when you're free? The isCancel API is widely used in interceptors to prevent reporting canceled requests. Now it won't pass the TS check.

image

@g1eny0ung
Copy link

g1eny0ung commented Jun 26, 2023

Also /cc @jasonsaayman @mzabriskie. Thanks for the effort you put into this.

@vladimirevstratov
Copy link

vladimirevstratov commented Jul 17, 2023

Hi! I have same issue
Снимок экрана 2023-07-17 в 17 41 34

So i need to put my cancel condition after all code in function, but it is not my desired solution.

Copy link

@samavati samavati left a comment

Choose a reason for hiding this comment

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

Let me know if this makes sense! I think it would be a nice improvement while keeping the changes minimal. Overall though, great work on the PR - appreciate you submitting the fix!

index.d.ts Outdated Show resolved Hide resolved
adimit and others added 2 commits October 25, 2023 12:01
Thanks to @samavati for the suggestion.

Co-authored-by: Ehsan Samavati <samaavaati@gmail.com>
@adimit
Copy link
Author

adimit commented Oct 25, 2023

Let me know if this makes sense! I think it would be a nice improvement while keeping the changes minimal. Overall though, great work on the PR - appreciate you submitting the fix!

Thanks for your suggestion! I've added it to the PR.

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.

Type of AxiosError gets narrowed down after isCancel check
4 participants