Make the LazyLoader.getJSON() helper invoke reject() in case of errors instead of resolving the promise to null

RESOLVED FIXED in 2.1 S5 (26sep)

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gsvelto, Assigned: mathieu, Mentored)

Tracking

unspecified
2.1 S5 (26sep)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(b2g-v2.2 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
+++ This bug was initially created as a clone of Bug #1062806 +++

While investigating replacing existing code with LazyLoader.getJSON() we came across cases where the original code was checking for the response status rather than the returned object. In this cases we currently resolve() the promise to a null value which is not very pretty.

To improve upon this behavior we should change the helper to reject() whenever the status is not 0 or 200 or if the returned object is null (we might check only the latter because IIRC all erroneous HTTP states will yield a null response). This way resolve() will always return a valid JSON object. This is more consistent with the idea of a promise returning successfully a valid value when the resolve() handler is called.

Once this is done existing useless null checks should also be removed.
(Assignee)

Comment 1

4 years ago
Created attachment 8492650 [details] [review]
Github PR for Bug 1069808 checking if status 0 and 200 then resolve else reject.

Hi Gabriele,

I just patched the lazy loader to implement the workaround we discussed earlier in bug 1062806.

I used the error object when we don't get the 200 or 0 status.

Thanks for taking a look into it !
Attachment #8492650 - Flags: feedback?(gsvelto)
(Assignee)

Comment 2

4 years ago
Hi,

I just ran Gaia-try because it didn't picked it up in the first commit of the PR.

Everything is green.

https://tbpl.mozilla.org/?rev=d40566685baf84555f098d6533d7848ada7ca71b&tree=Gaia-Try
(Reporter)

Comment 3

4 years ago
Comment on attachment 8492650 [details] [review]
Github PR for Bug 1069808 checking if status 0 and 200 then resolve else reject.

Some comments on the PR but in a nutshell it's better to check for the response field rather than the status since the response field never lies. Additionally you'll have to add a unit test loading an existing but malformed JSON file as well as a non existing JSON file and ensure that we call reject() in both cases instead of resolve().
Attachment #8492650 - Flags: feedback?(gsvelto) → feedback-
(Assignee)

Comment 4

4 years ago
Comment on attachment 8492650 [details] [review]
Github PR for Bug 1069808 checking if status 0 and 200 then resolve else reject.

I added the following things to PR.

1. Modified the check status instead checking for response null.
2. Added unit test for : Malformed/Invalid JSON and non existing json file.

Thanks for looking into it.
Attachment #8492650 - Flags: feedback?(gsvelto)
(Reporter)

Comment 5

4 years ago
Comment on attachment 8492650 [details] [review]
Github PR for Bug 1069808 checking if status 0 and 200 then resolve else reject.

Looking good, thanks.
Attachment #8492650 - Flags: feedback?(gsvelto)
Attachment #8492650 - Flags: feedback-
Attachment #8492650 - Flags: feedback+
(Assignee)

Comment 6

4 years ago
Comment on attachment 8492650 [details] [review]
Github PR for Bug 1069808 checking if status 0 and 200 then resolve else reject.

Added a Shared Gaia owner for review.
Attachment #8492650 - Flags: review?(timdream)
Comment on attachment 8492650 [details] [review]
Github PR for Bug 1069808 checking if status 0 and 200 then resolve else reject.

r+ since gsvelto have already f+'d the patch :)
Attachment #8492650 - Flags: review?(timdream) → review+
Assignee: nobody → mathieu
Status: NEW → ASSIGNED
Mathieu, if it's ready to land, please add "checkin-needed" in the keywords section on the bug :)
Please notice my comment on GitHub about the null value [1] before merging.

[1] https://github.com/mozilla-b2g/gaia/pull/24243#discussion-diff-17957392
(Reporter)

Comment 10

4 years ago
(In reply to Salvador de la Puente González [:salva] from comment #9)
> Please notice my comment on GitHub about the null value [1] before merging.
> 
> [1] https://github.com/mozilla-b2g/gaia/pull/24243#discussion-diff-17957392

You have a point, I didn't know that. However from a practical POV null needs to be special-cased in code whereas an empty object usually doesn't so I'd rather reject() all null values to simplify calling code. IMHO it's better this way.
(Assignee)

Comment 11

4 years ago
Alright then, merging it.
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/gaia/commit/737738b7d50ea3f39ff6b43036b25e488bed2ae2
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-b2g-v2.2: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S5 (26sep)
You need to log in before you can comment on or make changes to this bug.