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

call trackRequestEnd when fetch fails #119

Closed
ben-girardet opened this Issue Jan 11, 2019 · 8 comments

Comments

Projects
None yet
4 participants
@ben-girardet
Copy link
Contributor

ben-girardet commented Jan 11, 2019

I'm submitting a bug report

  • Library Version:
    1.7.0

Please tell us about your environment:

  • Operating System:
    OSX 10.14

  • Node Version:
    6.10.0

  • NPM Version:
    6.4.1
  • JSPM OR Webpack AND Version
    webpack 4
  • Browser:
    Chrome 71

  • Language:
    TypeScript 2.6.1

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:

  • What is the expected behavior?

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

  • What is the motivation / use case for changing the behavior?

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.

ben-girardet@380a47c

@huochunpeng

This comment has been minimized.

Copy link
Member

huochunpeng commented Jan 12, 2019

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?

return this.fetch(result);

@ben-girardet

This comment has been minimized.

Copy link
Contributor

ben-girardet commented Jan 14, 2019

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

This comment has been minimized.

Copy link
Member

bigopon commented Jan 14, 2019

@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

This comment has been minimized.

Copy link
Contributor

ben-girardet commented Jan 14, 2019

@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

This comment has been minimized.

Copy link
Member

bigopon commented Jan 14, 2019

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

@ben-girardet

This comment has been minimized.

Copy link
Contributor

ben-girardet commented Jan 14, 2019

ok thanks

@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Jan 15, 2019

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

@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Jan 15, 2019

Ok, TS PR is merged in :)

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