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

Refine arguments for error interceptors #2553

Closed
wants to merge 1 commit into from
Closed

Conversation

xamgore
Copy link

@xamgore xamgore commented Nov 15, 2019

Any is not really a good type for error handling. I believe they should be Request / Response objects.

@@ -120,7 +120,7 @@ export interface CancelTokenSource {
}

export interface AxiosInterceptorManager<V> {
use(onFulfilled?: (value: V) => V | Promise<V>, onRejected?: (error: any) => any): number;
use(onFulfilled?: (value: V) => V | Promise<V>, onRejected?: (error: V) => any): number;
Copy link
Collaborator

Choose a reason for hiding this comment

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

A successful callback should return V(AxiosResponse), but when there is an error, maybe it should return AxiosError insteadof V?

Copy link
Author

Choose a reason for hiding this comment

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

Are you sure about AxiosError?

@Alanscut
Copy link
Collaborator

Actually I think there will be no problem even if I don’t change it.

@chinesedfan
Copy link
Collaborator

Sorry, we don't know which type the error is. Because it can be thrown from anywhere.

@xamgore
Copy link
Author

xamgore commented Feb 10, 2020

@chinesedfan can we improve the type annotation by writing AxiosError | any? IDE would improve their intellisense significantly.

@chinesedfan
Copy link
Collaborator

@xamgore You can use a type assertion(#2013 (comment)) or contribute a type guard(#1863). In fact, I like type guard better.

@xamgore
Copy link
Author

xamgore commented Feb 10, 2020

Type assertion requires a hidden knowledge of AxiosError type existence. Same with a type guard — you have to read about it in docs or the source code before usage.

On the other hand if onRejected accepts a union, people will be able to use optional chaining, and IDE will enumerate its inner fields:

export interface AxiosInterceptorManager<V> {
-  use(onFulfilled?: (value: V) => V | Promise<V>, onRejected?: (error: V) => any): number;
+  use(onFulfilled?: (value: V) => V | Promise<V>, onRejected?: (error: AxiosError | any) => any): number;
  eject(id: number): void;
}

@chinesedfan
Copy link
Collaborator

@xamgore AxiosError | any looks making sense, though it seems to be weird at first glance. Can you find some documents or example links about it? Then we can persuade more people.

@xamgore
Copy link
Author

xamgore commented Feb 10, 2020

Typescript documentation has a similar case:

They define two interfaces, possibly intersecting:

interface Bird {
    fly();
    layEggs();
}

interface Fish {
    swim();
    layEggs();
}

function getSmallPet(): Fish | Bird {
    // ...
}

let pet = getSmallPet();
pet.layEggs(); // okay
pet.swim();    // errors

pet?.layEggs(); // okay
pet?.swim(); // okay, though swim() wasn't called

Then they provide a user type-guard:

function isFish(pet: Fish | Bird): pet is Fish {
    return (pet as Fish).swim !== undefined;
}

if (isFish(pet)) pet.swim() // ok

In our case Fish | Bird === AxiosError | any.

@chinesedfan
Copy link
Collaborator

I know that official example. And just thought any as a bit special. Thanks for your work and I will do some further investigating. Also waiting for others' opinions in #2729.

@ahuglajbclajep
Copy link

Using asserts x is T added in TypeScript 3.7, you might be able to write as follows.

function assertIsAxiosError<T>(err: any): asserts err is AxiosError<T> {
  if (err.isAxiosError) {
    throw new AssertionError("Not an AxiosError!");
  }
}

// in onRejected function
// type Data = { x: number };
try {
  assertIsAxiosError<Data>(err);
  console.log(err.response?.data.x); // err: AxiosError<Data>
} catch {
  console.log(err); // err: any
}

I would be happy if this information is helpful for you.

@axios axios locked and limited conversation to collaborators May 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants