search

aurelia / fetch-client

An HTTP Client based on the Fetch standard.
MIT License
76 stars 47 forks source link

call trackRequestEnd when fetch fails #119

Closed ben-girardet closed 5 years ago

ben-girardet commented 5 years ago

I'm submitting a bug report

Please tell us about your environment:

Current behavior: I'm using the isRequesting observable property of fetch-client to provide user feedback in my app when a request is in progress. It works well, except when the server I'm trying to reach with fetch-client is down. Then, the fetch-client rejects the request with "Failed to fetch" error but do not set isRequesting to false.

Expected/desired behavior:

When the request fails, I think isRequesting must be set accordingly

The goal of isRequesting is to know if a request is in progress. If the requests fails, it's not in progress anymore.

My attempt to fix this

I'm assuming that the trackRequestEnd is not called when fetch rejects. I've tried to fork the code and build the fetch-client library but for some reason I'm not successful at building the code. I provide here my supposed fix for this scenario, if anybody can help out with building / testing this, it would be great.

https://github.com/ben-girardet/fetch-client/commit/380a47c3967a3139fcdf7d35044f908dea50f9ef

3cp commented 5 years ago

Looks like a valid fix to me. PR pls.

Your added catch after then might calls trackRequestEnd twice. Need some test coverage to govern trackRequestEnd is called only once (it can only been called once, because it mutates a counter).

Put error handle in the same then call may fix the problem.

.then(
  result => {
        if (Request.prototype.isPrototypeOf(result)) {
// this recursive fetch call might already triggered trackRequestEnd in an error catch
// you don't want to process the error again that triggers another trackRequestEnd
          return this.fetch(result);
        }
        this::trackRequestEnd();
        return result;
  },
  error => {
    this::trackRequestEnd();
    return Promise.reject(error);
  }
);

@EisenbergEffect I am not familiar with this. It looks every remote fetching need one pair of trackRequestStart and trackRequestEnd. But the implementation uses recursive fetch call, would not this introduce some subtle bug?

The one in line 142 skips a trackRequestEnd, would not it result an unpaired trackRequestStart?

https://github.com/aurelia/fetch-client/blob/edc826fdfe5ea4d09765a9a89816091a23916d1f/src/http-client.js#L142

ben-girardet commented 5 years ago

I would be happy to somehow help out with this issue but as stated before I currently am unable to build the code source locally. Here is my config and errors, can anybody help me out ?

node -v
v6.10.0

npm -v
6.4.1

gulp -v
CLI version 2.0.1
Local version 3.9.1

I forked and cloned the repo locally + ran npm install and then:

gulp build
[13:04:15] Using gulpfile ~/web/fetch-client/gulpfile.js
[13:04:15] Starting 'build'...
[13:04:15] Starting 'clean'...
[13:04:15] Finished 'clean' after 42 ms
[13:04:15] Starting 'build-index'...
[13:04:16] Finished 'build-index' after 43 ms
[13:04:16] Starting 'build-babel-es2015'...
[13:04:16] Starting 'build-babel-commonjs'...
[13:04:16] Starting 'build-babel-amd'...
[13:04:16] Starting 'build-babel-system'...
[13:04:16] Starting 'build-babel-native-modules'...

events.js:160
      throw er; // Unhandled 'error' event
      ^
Error: /.../fetch-client/dist/aurelia-fetch-client.js: Unsupported type annotation type: NullLiteralTypeAnnotation
    at getTypeAnnotationString (/.../fetch-client/node_modules/babel-dts-generator/lib/generators/dts.js:745:13)
bigopon commented 5 years ago

@ben-girardet I've created a TS conversion at #120 . It would be great if you could help continue the work based on that. It would make development easier

ben-girardet commented 5 years ago

@bigopon would love to give it a try based on your TS version. What's the process for that? Should I fork from your fork ? Or wait for your PR to be merged before working on this ?

bigopon commented 5 years ago

@ben-girardet If @EisenbergEffect can do the review-merge soon, I think it's better to wait a bit 😆

ben-girardet commented 5 years ago

ok thanks

EisenbergEffect commented 5 years ago

It's up next on my list. Thanks for your patience :) Should be tonight or tomorrow at latest. Hopefully tonight.

EisenbergEffect commented 5 years ago

Ok, TS PR is merged in :)