janhommes / o.js

o.js - client side oData lib.
https://janhommes.github.io/o.js/example/
MIT License
238 stars 57 forks source link

`onError` is not called when a network error occurs in `query` and `batch` methods #141

Closed xel1045 closed 1 year ago

xel1045 commented 1 year ago

When a network error occurs, a TypeError is thrown and the fetch method passes it to the onError handler, but query doesn't. In order to be able to handle all possible errors, I guess the behaviour should be aligned between the two methods.

Here how the catch looks like in the three methods:

  public async query(query?: OdataQuery) {
    try {
      // ...
    } catch (ex) {
      throw ex;
    } finally {
      this.requests = [];
    }
  }

  public async fetch(query?: OdataQuery) {
    try {
      // ...
    } catch (ex) {
      this.config.onError(this, ex);
      throw ex;
    } finally {
      // ...
    }
  }

  public async batch(query?: OdataQuery) {
    try {
      // ...
    } catch (ex) {
      throw ex;
    } finally {
      // ...
    }
  }

Are you open to a PR that fixes this? Thanks!

janhommes commented 1 year ago

Yeah, it is thrown here: https://github.com/janhommes/o.js/blob/master/src/OHandler.ts#L56-L57

Everything above 400 will call this method. Do you have an example where this does not work?

xel1045 commented 1 year ago

It's not called for network errors, so if you block the API call in your browser's console, fetch will throw a TypeError and the onError won't be called.

  public async query(query?: OdataQuery) {
    try {
      this.config.onStart(this);
      const response: Response[] = await this.getFetch(query); // <------ A TypeError is thrown when the API cannot be reached
      const json = await Promise.all(
        response.map(
          async (res) => {
            if (res.status >= 400) {
              this.config.onError(this, res);
              throw res;
            } else if (res.ok && res.json) {
              try {
                this.config.onFinish(this, res);
                const data = await res.json();
                return data[this.config.fragment] || data;
              } catch (ex) {
                return res;
              }
            } else {
              return await res.text();
            }
          },
        ),
      );
      return json.length > 1 ? json : json[0];
    } catch (ex) {
      throw ex; // <---------- The TypeError is rethrown without calling onError
    } finally {
      this.requests = [];
    }
  }

To have a similar behaviour to the fetch, this method could be updated like this:

  public async query(query?: OdataQuery) {
    try {
      this.config.onStart(this);
      const response: Response[] = await this.getFetch(query);
      const json = await Promise.all(
        response.map(
          async (res) => {
            if (res.status >= 400) {
              throw res;
            } else if (res.ok && res.json) {
              try {
                this.config.onFinish(this, res);
                const data = await res.json();
                return data[this.config.fragment] || data;
              } catch (ex) {
                return res;
              }
            } else {
              return await res.text();
            }
          },
        ),
      );
      return json.length > 1 ? json : json[0];
    } catch (ex) {
      this.config.onError(this, ex);
      throw ex;
    } finally {
      this.requests = [];
    }
  }
janhommes commented 1 year ago

hi, yeah might be a good improvement. I would accept a PR for this.