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

AxiosError type for javascript #1415

Closed
vladkolotvin opened this issue Mar 14, 2018 · 37 comments
Closed

AxiosError type for javascript #1415

vladkolotvin opened this issue Mar 14, 2018 · 37 comments

Comments

@vladkolotvin
Copy link

vladkolotvin commented Mar 14, 2018

Summary

Axios rejects error with Error type. Adding custom error type will help with distinguishing between axios and other errors. The only way now is to check 'request' field existence which is not always possible. Our internal custom error type has this field too.

Example:

const { axios, AxiosError } = require('axios');
...
try {
  ...
  const response = await axios.get(url);
  ...
} catch (error) {
  if(error instanceof AxiosError) {
    return processAxiosError(error);
  }
  throw error;
}

Context

  • axios version: 0.18.0

Relates to: #24

@AyushG3112
Copy link
Contributor

This would be simple enough to do, but would be a breaking change.

@vladkolotvin
Copy link
Author

AxiosError has to inherit from Error. This wouldn't breaking change.

@AyushG3112
Copy link
Contributor

It would be if someone is checking error.constructor.name === 'Error'

@vladkolotvin
Copy link
Author

I agree but this is a minor breaking change. Can we sacrifice this for better API?

@AyushG3112
Copy link
Contributor

AyushG3112 commented Mar 15, 2018

I'm in favor of this, but personally I would prefer, if the maintainers agree ofcourse, that we bump axios to a semver, v1.0.0

Axios certainly seems stable enough for that, and following semver-major prevents breakage.

On another note, given the widespread use of axios and the preferred HTTP library of React and Vue.js, a breaking change can have major impacts which need to be considered.

@AyushG3112
Copy link
Contributor

AyushG3112 commented Mar 15, 2018

Actually, now that I checked, axios is working on a v1.0.0 branch, I might open a PR for this today.

@vladkolotvin
Copy link
Author

@mzabriskie, @nickuraltsev

@vladkolotvin
Copy link
Author

@JustinBeckwith

@AyushG3112
Copy link
Contributor

/cc @Khaledgarbaya @axe312ger @zcei

@axe312ger
Copy link
Contributor

Ohh yes, this would help a lot. Having kinda the same issue in our SDK.

Can we add this to #1333 @Khaledgarbaya?

@AyushG3112
Copy link
Contributor

The PR is already there, I added a soft fix for the 0.x versions as a flag in #1419, and the Error type change for the 1.x in #1420, refer the linked PRs

@AyushG3112
Copy link
Contributor

@axe312ger @Khaledgarbaya any updates on this?

@AyushG3112
Copy link
Contributor

AyushG3112 commented Apr 28, 2018

@axe312ger any update please?

@axe312ger
Copy link
Contributor

Have to pass on to @Khaledgarbaya 😅

@seanlindo
Copy link

This would be wonderful. Running into this issue now!

@nilptr
Copy link

nilptr commented Jul 5, 2018

Is there any progress on this issue? It's useful to distinguish axios errors from other errors. And adding isAxiosError flag to errors thrown by axios #1419 is a good solution without breaking changes. Consider adding this flag in a patch version please?

@fractalzombie
Copy link

+1

@vladkolotvin
Copy link
Author

@Khaledgarbaya any updates on this?

@axe312ger
Copy link
Contributor

I still like this, as we all have jobs, feel free to contribute that as PR

@vladkolotvin
Copy link
Author

vladkolotvin commented Aug 20, 2018

@axe312ger We already have 2 PRs for 5 months!
#1420 and #1419

@axe312ger
Copy link
Contributor

Oops. Hard to keep track on this repo.

I'd prefer #1419 to end up in master since the v1 rewrite currently is kinda frozen due to lack of time on our side.

Can you resolve conflicts and mention some people in there?

@Khaledgarbaya
Copy link
Collaborator

This is now fixed in #1419

@axe312ger
Copy link
Contributor

That was fast :)

@vladkolotvin
Copy link
Author

That was open source:)

@axe312ger
Copy link
Contributor

The power of GitHub mentions :trollface:

@AyushG3112
Copy link
Contributor

@axe312ger @Khaledgarbaya is it too soon to request a release please?

@Khaledgarbaya
Copy link
Collaborator

@AyushG3112 Unfortunately I don't have npm release rights

@skaldo
Copy link

skaldo commented Oct 18, 2018

@nickuraltsev @emilyemorehouse is there a plan releasing this anytime soon?

@fractalzombie
Copy link

I think if it is implemented, it will be more practical and clearer than using <Error.isAxiosError>.

@AyushG3112
Copy link
Contributor

@axe312ger @Khaledgarbaya @nickuraltsev @emilyemorehouse can any of you please @-mention someone with npm publish rights?

@rhyek
Copy link

rhyek commented Jan 25, 2019

AxiosError would've been nicer... Can someone publish the isAxiosError change, please?

@ypresto
Copy link

ypresto commented Apr 1, 2019

Because at least we need some type-guard-aware function like function isAxiosError(error: Error): error is AxiosError { return !!error.isAxiosError }, it might be better to have it in library side.

@AyushG3112
Copy link
Contributor

@Khaledgarbaya @axe312ger It's been months, can we have a release please?

@axe312ger
Copy link
Contributor

We have no rights to publish axios to npm

@AyushG3112
Copy link
Contributor

Could you tag someone who does please?

@zcei
Copy link
Contributor

zcei commented May 17, 2019

You did yourself last December #1415 (comment) - Nick and Emily have publish rights.

@emilyemorehouse
Copy link
Member

👋 I've been handling all releases for consistency. As soon as CI is green again, I'll release!

@axios axios locked and limited conversation to collaborators May 22, 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

No branches or pull requests