feat(Cache): better error handling by NathanWalker · Pull Request #6458 · NativeScript/NativeScript · GitHub

PR Checklist

The PR title follows our guidelines: https://github.com/NativeScript/NativeScript/blob/master/CONTRIBUTING.md#commit-messages.
You have signed the CLA.
All existing tests are passing: https://github.com/NativeScript/NativeScript/blob/master/DevelopmentWorkflow.md#running-unit-tests.
Tests for the changes are included - https://github.com/NativeScript/NativeScript/blob/master/WritingUnitTests.md.

What is the current behavior?

Usage of the image Cache class can result in many app problems due to invalid error handling conditions.

What is the new behavior?

Proper error handling can be wired up to circumvent troubling app conditions, ie:

this._cache.push((<DownloadRequest> {
  key: url,
  url: url,
  completed: (image: any, key: string) => {                                                       
    if (url === key) {
       // handle success
    }
  },
  error: (key: string) => {
    if (url === key) {
      // we can now properly handle error for failed url request here
    }
  }
}));

These changes are dependent on widgets PR here: NativeScript/tns-core-modules-widgets#147

This helps avoid these app crashes as well:

System.err: StackTrace:
System.err: Frame: function:'push.../node_modules/tns-core-modules/ui/image-cache/image-cache.js.Cache.set', file:'file:///data/data/org.nativescript.app/files/app/vendor.js', line: 134088, column: 21

Hi and thanks for the PR!

The team have been quite busy baking the upcoming NativeScript 5.0 release. We hope you love it as much as we do! The major release is the reason we taking longer with PR reviews. We will resume active reviewing in the following weeks.

Thanks for your patience and for being awesome contributor!

Hey @NathanWalker,

Let's get merged the related pull request in tns-core-modules-widgets to facilitate the work here.

Could you also share how to reproduce the errors you experience?

Thank you!

Hey @NathanWalker,

A few things:

the build of the tns-core-moduels package fails with:

tns-core-modules/ui/image-cache/image-cache.android.ts(56,13): error TS2345: Argument of type '{ onComplete: (result: any, context: any) => void; onError: (context: any) => void; }' is not assignable to parameter of type 'ICompleteCallback'.
  Object literal may only specify known properties, and 'onError' does not exist in type 'ICompleteCallback'.

The tns-platform-declarations/android/org.nativescript.widgets.d.ts needs to be updated with the changes in NativeScript/tns-core-modules-widgets#147 and NativeScript/tns-core-modules-widgets#149:

onError(err: string, context: Object): void;

also the http-request.android.ts module needs to be updated with onError.

tns-core-modules/http/http-request/http-request.android.ts(51,76): error TS2345: Argument of type '{ onComplete: (result: any, context: any) => void; }' is not assignable to parameter of type 'ICompleteCallback'.
  Property 'onError' is missing in type '{ onComplete: (result: any, context: any) => void; }'.

Hey @NathanWalker,

I committed a few updates on the API and implementation that we think are required and would expect in future pull requests.

Could you please handle my comments above to proceed forward?

Thank you!

Hey @NathanWalker, is there any update here?

Add DownloadError interface.

Use arrow functions. Remove an unnecessary `trace.write().

Update Android `constructor()`. Move `key` and `image` check to `set()`. Update `trace.write`.

@vchimev ok got this updated now. The changes are based on this PR which I completely agree with:
NativeScript/tns-core-modules-widgets#149

Let me know if you need anything else here.

test

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

ghost added the ♥ community PR label Oct 25, 2018

ns-bot added the cla: yes label Oct 25, 2018

vakrilov assigned ADjenkov and vchimev and unassigned ADjenkov Nov 20, 2018

vchimev suggested changes Nov 29, 2018

View reviewed changes

ghost added in progress and removed ♥ community PR labels Nov 30, 2018

vchimev force-pushed the feature/image-cache-error-handling branch from 376b6b1 to a05a1fb Compare November 30, 2018 16:04

NathanWalker and others added 4 commits December 11, 2018 10:17

feat(Cache): better error handling

ad1f211

refactor(image-cache): add error parameter to _onDownloadError

76511a1

Add DownloadError interface.

refactor(image-cache): updates for iOS

c8ded5f

Use arrow functions. Remove an unnecessary `trace.write().

refactor(image-cache): updates for Android

5701d21

Update Android `constructor()`. Move `key` and `image` check to `set()`. Update `trace.write`.

NathanWalker force-pushed the feature/image-cache-error-handling branch from a05a1fb to 5701d21 Compare December 11, 2018 18:17

fix(image-cache): onError handling

974ca28

vchimev approved these changes Dec 12, 2018

View reviewed changes

Merge branch 'master' into feature/image-cache-error-handling

0d81a4a

Merge branch 'master' into feature/image-cache-error-handling

b3c83aa

ghost assigned SvetoslavTsenov Dec 14, 2018

vchimev closed this Dec 16, 2018

ghost removed the in progress label Dec 16, 2018

vchimev reopened this Dec 17, 2018

ghost added the in progress label Dec 17, 2018

vchimev merged commit 3481e6f into NativeScript:master Dec 17, 2018

ghost removed the in progress label Dec 17, 2018

lock Bot locked and limited conversation to collaborators Dec 17, 2019

feat(Cache): better error handling by NathanWalker · Pull Request #6458 · NativeScript/NativeScript · GitHub

Uh oh!

feat(Cache): better error handling - #6458

feat(Cache): better error handling#6458
vchimev merged 7 commits into
NativeScript:masterfrom
NathanWalker:feature/image-cache-error-handling

NathanWalker commented Oct 25, 2018

Uh oh!

NathanWalker commented Oct 25, 2018 •
edited
Loading

Uh oh!

Uh oh!

vakrilov commented Nov 2, 2018

Uh oh!

vchimev commented Nov 26, 2018

Uh oh!

vchimev left a comment •
edited
Loading

Uh oh!

Uh oh!

vchimev commented Nov 30, 2018 •
edited
Loading

Uh oh!

Uh oh!

vchimev commented Dec 11, 2018

Uh oh!

NathanWalker commented Dec 11, 2018

Uh oh!

vchimev commented Dec 12, 2018

Uh oh!

SvetoslavTsenov commented Dec 14, 2018

Uh oh!

lock Bot commented Dec 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

feat(Cache): better error handling by NathanWalker · Pull Request #6458 · NativeScript/NativeScript · GitHub

Uh oh!

feat(Cache): better error handling - #6458

feat(Cache): better error handling#6458vchimev merged 7 commits intoNativeScript:masterNativeScript/NativeScript:masterfrom NathanWalker:feature/image-cache-error-handlingNathanWalker/NativeScript:feature/image-cache-error-handlingCopy head branch name to clipboard

Conversation

NathanWalker commented Oct 25, 2018

PR Checklist

What is the current behavior?

What is the new behavior?

Uh oh!

NathanWalker commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vakrilov commented Nov 2, 2018

Uh oh!

vchimev commented Nov 26, 2018

Uh oh!

vchimev left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vchimev commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vchimev commented Dec 11, 2018

Uh oh!

NathanWalker commented Dec 11, 2018

Uh oh!

vchimev commented Dec 12, 2018

Uh oh!

SvetoslavTsenov commented Dec 14, 2018

Uh oh!

lock Bot commented Dec 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

feat(Cache): better error handling#6458
vchimev merged 7 commits into
NativeScript:masterfrom
NathanWalker:feature/image-cache-error-handling

NathanWalker commented Oct 25, 2018 •
edited
Loading

vchimev left a comment •
edited
Loading

vchimev commented Nov 30, 2018 •
edited
Loading