Callback with error if error thrown due to network

[cheedep]

The Fetch API throws TyperError if there are network issues. fixes #558 #389

[codecov-io]

Codecov Report

Merging #605 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #605   +/-   ##
=======================================
  Coverage   87.28%   87.28%           
=======================================
  Files          72       72           
  Lines        3428     3428           
  Branches      652      652           
=======================================
  Hits         2992     2992           
  Misses        419      419           
  Partials       17       17

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16d1a1a...26e0f08. Read the comment docs.

[mlabieniec]

should we check here if error is actually a TypeError?

if (err && err instanceof TypeError)

for example: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch

[cheedep]

I think that would make sense if a the callback is called with a new error with a more meaningful message. but since most clients possibly handle the same error when directly using the fetch, I thought this is better.

[cheedep]

I see what you mean. Should we also be checking for the error message?

[mlabieniec]

maybe so we have a uniform approach and 1 return we could do something like:

let code = 'UnknownError', error = {};

// first check if we have a service error
if ( response && response.headers && response.headers.get('x-amzn-errortype') ) {
   try {
       code = (response.headers.get('x-amz-errortype')).split(':')[0];
       error = {
          code,
          name: code,
          statusCode: response.status,
          message: (response.status) ? response.status.toString() : null,
        };
   } catch( error ) { 
      // pass through so it doesn't get swallowed if we can't parse it 
   }
// otherwise check error
} else if ( err && instanceof TypeError ) {
    error = {
        code: err.name,
        name: err.name,
        message: err.message
    };
// finally case will return 'UnknownError'
} else {
    error = { code };
}
return callback(error);

[cheedep]

I think this is much better. How about providing onRejected handler to the then and throwing an error with a better message. Example:

fetch(this.endpoint, options)
      .then(resp => {
        response = resp;
        return resp;
      }, err => {
        // If error happens here, the request failed and if it is type error throw network error
        if (err instanceof TypeError) {
          throw new Error('Network error');
        }
        throw err;
      })

[cheedep]

Hi @mlabieniec , what are your thoughts about the fix?

[lukezbikowski]

👍 Any chance to merge it soon please? :-) This issue is very problematic (especially for mobile apps). There is no way to catch that error (unless I missed something)... the only workarround we have is downgrade of amazon-cognito-identity-js ...

[mlabieniec]

this should probably return:
return throw new Error('Network error');

or run an } else {

[SupremeTechnopriest]

Throwing an error automatically returns control to the caller. You dont need return or a branch here.

[stripathix]

When will this fix be live on npm?

[github-actions]

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.