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

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

Closed
xel1045 opened this issue Dec 5, 2022 · 3 comments · Fixed by #143
Closed

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

xel1045 opened this issue Dec 5, 2022 · 3 comments · Fixed by #143
Labels

Comments

@xel1045
Copy link
Contributor

xel1045 commented Dec 5, 2022

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!

@xel1045 xel1045 changed the title onError is not called when a network error occurs using the query and batch methods onError is not called when a network error occurs in query and batch methods Dec 5, 2022
@janhommes janhommes added the bug label Dec 9, 2022
@janhommes
Copy link
Owner

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
Copy link
Contributor Author

xel1045 commented Dec 9, 2022

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
Copy link
Owner

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

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

Successfully merging a pull request may close this issue.

2 participants