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

Feature: recognize a Wretch error when catching request failure #79

Closed
SamyCookie opened this issue Jun 17, 2020 · 13 comments
Closed

Feature: recognize a Wretch error when catching request failure #79

SamyCookie opened this issue Jun 17, 2020 · 13 comments

Comments

@SamyCookie
Copy link

SamyCookie commented Jun 17, 2020

Hi !

Today when having a request error, an error object can be reached.
cf.

const err = new Error(msg)

But if the request is embedded inside a ton of wrappers, when the error is catched, it is complicated to know if the error come from a wretch error or anything else.

I just propose to create a custom Wretch error and make it exportable inside API, eg.

export WretchError extends Error {
  constructor(msg, response) {
     super(msg);
     this.response = response;
  }
  get status() {
    return this.response.status;
  }
}

// inside in resolver.ts

 const err = new WretchError(msg, response);
 err[conf.errorType || "text"] = msg

and in user code, we can do this:

import { WretchError } from 'wretch.js';

async function requestWithWretchAndOtherStuff() { [...] }

try {
  const foo = await requestWithWretchAndOtherStuff()
} except(err) {
  if (err instanceof WretchError) {
     // process request issue
  } else {
    // process other stuff not request related to request issue
  }
}

What do you think ?

@elbywan
Copy link
Owner

elbywan commented Jun 21, 2020

Hi @SamyCookie, thanks for the suggestion!

I just propose to create a custom Wretch error and make it exportable inside API, eg.

Well, I think that it would be a good idea on paper!

Unfortunately, I'm a bit worried by the fact that it will add more code and bump the package size. And my goal is to go through the codebase soon and try to remove as much bytes as I can.

In addition, there is actually an easy way to check for an error thrown by wretch already (even though it is not ideal):

if(err.status && err.response) {
   // an error thrown by wretch has this two properties
}

@SamyCookie
Copy link
Author

SamyCookie commented Jun 22, 2020

Ok, but I think that this solution is less clean and usable, and maybe should be documented if kept as is.

A smaller proposition is possible:

--- a/src/resolver.ts
+++ b/src/resolver.ts
@@ -34,6 +34,7 @@ export type ResponseChain = {
 class WretchErrorWrapper {
     constructor(public error: any) {}
 }
+export class WretchError extends Error {}
 
 export const resolver = (wretcher: Wretcher) => {
     const {
@@ -71,7 +72,7 @@ export const resolver = (wretcher: Wretcher) => {
             if (!response.ok) {
                 return response[conf.errorType || "text"]().then(msg => {
                     // Enhances the error object
-                    const err = new Error(msg)
+                    const err = new WretchError(msg)
                     err[conf.errorType || "text"] = msg
                     err["status"] = response.status
                     err["response"] = response

And I don't think it adds too much bytes, especially if minified.

@elbywan
Copy link
Owner

elbywan commented Jun 25, 2020

Ok, but I think that this solution is less clean and usable, and maybe should be documented if kept as is.

It is already documented here:

type WretcherError = Error & { status: number, response: WretcherResponse, text?: string, json?: Object }

A smaller proposition is possible:

I am a bit busy with other projects these days, but I'll think about it. In any case the library is ~3Kb minzipped as of today, which is already too much.

@SamyCookie
Copy link
Author

It is already documented here:

type WretcherError = Error & { status: number, response: WretcherResponse, text?: string, json?: Object }

Oh, how I did to not see this ? Sorry !

I am a bit busy with other projects these days, but I'll think about it. In any case the library is ~3Kb minzipped as of today, which is already too much.

Thanks for your time !

elbywan added a commit that referenced this issue Jul 3, 2022
@elbywan elbywan mentioned this issue Jul 3, 2022
@elbywan
Copy link
Owner

elbywan commented Aug 1, 2022

Should be fixed with the v2.

@elbywan elbywan closed this as completed Aug 1, 2022
@SamyCookie
Copy link
Author

Thanks you for this code integration, but I do not see any way how I can reach the WretchError type from outside the main wretch code ?

@elbywan
Copy link
Owner

elbywan commented Aug 1, 2022

Thanks you for this code integration, but I do not see any way how I can reach the WretchError type from outside the main wretch code ?

The type should be imported from the root:

import type { WretchError } from "wretch"

The class can be imported from the resolver:

import { WretchError } from "wretch/resolver"

@SamyCookie
Copy link
Author

Maybe I have missed something, but the WretchError you described are typescript objects. However I tried to get the WretchError class from a JavaScript ESM built file, which is impossible because WretchError seems not to be exported in the ESM module.

In the dist bundle wretch.all.min.mjs, the corresponding WretchError is generated as this : class s extends Error{}. s does not seems to be exported nowhere. Is this an intended behaviour ?

@elbywan
Copy link
Owner

elbywan commented Aug 16, 2022

Maybe I have missed something, but the WretchError you described are typescript objects

No, the second one is the javascript Class and is exported from wretch/resolver.

In the dist bundle wretch.all.min.mjs, the corresponding WretchError is generated as this : class s extends Error{}. s does not seems to be exported nowhere. Is this an intended behaviour ?

Ah, I was not aware that you were using the bundles through script tags.
I will push a commit to export WretchError from the root index, which should make it accessible like this:

<script type="module">
import wretch from 'https://cdn.skypack.dev/wretch/dist/bundle/wretch.all.min.mjs'

console.log(wretch.WretchError) // class s {}

wretch("http://httpstat.us/400").get().badRequest(error => {
  console.log(error instanceof Error) // true
  console.log(error instanceof wretch.WretchError) //true
}).res()
</script>

elbywan added a commit that referenced this issue Aug 16, 2022
@dxptqhtlutehvlyxcmtg
Copy link

Would it be possible for the WretchError class to define the same properties as the WretchError type? Currently type narrowing doesn't really work since the extra properties of the WretchError are attached dynamically to the error instance. Consequently, TS isn't aware of them:

import wretch from 'wretch';

try {
  // throws
} catch(e) {
  if(e instanceof wretch.WretchError) {
      console.log(e.response); // TS: 'response' does not exist
  }
}

Or perhaps there's some declaration merging going on that I'm missing?

I'm on 2.0.3 by the way

@elbywan
Copy link
Owner

elbywan commented Aug 18, 2022

Hey @dxptqhtlutehvlyxcmtg 👋,

Would it be possible for the WretchError class to define the same properties as the WretchError type

I just released 2.0.4 📦 which contains a rework of the WretchError type, it is now an interface and the js class declares the extra fields. I'm pretty confident it should solve the issue 😉.

@SamyCookie
Copy link
Author

The main goal of the current ticket I wrote was solve with the 2.0.3 version for me, thanks for the work !

@dxptqhtlutehvlyxcmtg
Copy link

Hey @dxptqhtlutehvlyxcmtg wave,

Would it be possible for the WretchError class to define the same properties as the WretchError type

I just released 2.0.4 package which contains a rework of the WretchError type, it is now an interface and the js class declares the extra fields. I'm pretty confident it should solve the issue wink.

Type narrowing works great now. Thank you very much

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants