1062806 - Use the new shared helper for loading JSON files in the cost control app

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Cost Control, defect)

Description

[Gabriele Svelto [:gsvelto]]

+++ This bug was initially created as a clone of Bug #990047 +++

We can replace the custom code used to load JSON file here with the new shared helper:

https://github.com/mozilla-b2g/gaia/blob/cc9fa7ed829b6af23606b41a9e5797866b0a3316/apps/costcontrol/js/config/config_manager.js#L117

Comment 1

[Gaurav Mittal (gauravmittal1995)]

Hey, I would like to work on this bug!! Can you please assign it to me?
And do i have to replace the above said function with the LazyLoader?

Comment 2

[Gabriele Svelto [:gsvelto]]

(In reply to gauravmittal from comment #1)
> Hey, I would like to work on this bug!! Can you please assign it to me?
> And do i have to replace the above said function with the LazyLoader?

You've already been assigned bug 1062783 so it's better if you finish that one first before being assigned to another bug.

Comment 3

[Mathieu Rhéaume]

Attachment: Github PR for Bug 1062806 Using LazyLoader and Premises

Hi,

Working on some more blocks for the LazyLoader ticket.

I used the good and fail premise instead of checking the xhr status as if fetch fail it'll raise the error premise :).

UI Tests passed.
Lint passed.

I indented with 2 spaces like the other files.

Thanks for reviewing !

Comment 4

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8491072 [details] [review]
Github PR for Bug 1062806 Using LazyLoader and Premises

Pretty straightforward, LGTM.

Comment 5

[Salvador de la Puente González [:salva]]

Grabriel, in current `LazyLoad.getJSON()` I found that passing an invalid or non-existent URL will resolve the promise with `null`. You've this commented in the function so I assume it's an intended behavior but this supposes, in many cases, to check for `result !== null` and probably to repeat the rejection code for this case.

What do you think?

Comment 6

[Salvador de la Puente González [:salva]]

Comment on attachment 8491072 [details] [review]
Github PR for Bug 1062806 Using LazyLoader and Premises

Please Gabriel, ask for my review again when you answer me. Thank you.

Comment 7

[Gabriele Svelto [:gsvelto]]

(In reply to Salvador de la Puente González [:salva] from comment #5)
> Grabriel, in current `LazyLoad.getJSON()` I found that passing an invalid or
> non-existent URL will resolve the promise with `null`. You've this commented
> in the function so I assume it's an intended behavior but this supposes, in
> many cases, to check for `result !== null` and probably to repeat the
> rejection code for this case.
> 
> What do you think?

When the function was written I had a good look around the code in gaia and most of it didn't care about the response. It seems that in most places we'd rather check the XMLHttpRequest 'response' field against null rather than look at the return code and acting on that hence the decision to write the method that way.

That being said I think it would be cleaner from a high-level perspective to reject() in the cases you mention (e.g. malformed JSON code, 4xx status codes, etc...) so I'm OK to open a bug about that purpose. However there's one thing that should be taken care of if we follow that road: ensuring that existing uses are compatible with it and removing existing useless null checks (plus changing the in-code documentation to explicitly say that resolve() will always return a valid object and all other cases will be reject()ed with an appropriate error code).

Comment 8

[Salvador de la Puente González [:salva]]

(In reply to Gabriele Svelto [:gsvelto] from comment #7)
> (In reply to Salvador de la Puente González [:salva] from comment #5)
> > Grabriel, in current `LazyLoad.getJSON()` I found that passing an invalid or
> > non-existent URL will resolve the promise with `null`. You've this commented
> > in the function so I assume it's an intended behavior but this supposes, in
> > many cases, to check for `result !== null` and probably to repeat the
> > rejection code for this case.
> > 
> > What do you think?
> 
> When the function was written I had a good look around the code in gaia and
> most of it didn't care about the response. It seems that in most places we'd
> rather check the XMLHttpRequest 'response' field against null rather than
> look at the return code and acting on that hence the decision to write the
> method that way.
> 
> That being said I think it would be cleaner from a high-level perspective to
> reject() in the cases you mention (e.g. malformed JSON code, 4xx status
> codes, etc...) so I'm OK to open a bug about that purpose. However there's
> one thing that should be taken care of if we follow that road: ensuring that
> existing uses are compatible with it and removing existing useless null
> checks (plus changing the in-code documentation to explicitly say that
> resolve() will always return a valid object and all other cases will be
> reject()ed with an appropriate error code).

Yep. Indeed, a simply `null` value inside a JSON object it's perfectly valid [1] so acting this way we can totally distinguish between a successful retrieve and failure cases.

[1] http://www.json.org/

Comment 9

[Salvador de la Puente González [:salva]]

Comment on attachment 8491072 [details] [review]
Github PR for Bug 1062806 Using LazyLoader and Premises

It's ok for me but before merging address the problem on GitHub. I repeat here:

 1.- Let's file a bug for changing `getJSON()` implementation.
 2.- Let's add the null check now.
 3.- Leave a TODO comment about remove when the bug is fixed.

Comment 10

[Salvador de la Puente González [:salva]]

(In reply to Gabriele Svelto [:gsvelto] from comment #7)
> That being said I think it would be cleaner from a high-level perspective to
> reject() in the cases you mention (e.g. malformed JSON code, 4xx status
> codes, etc...) so I'm OK to open a bug about that purpose. However there's
> one thing that should be taken care of if we follow that road: ensuring that
> existing uses are compatible with it and removing existing useless null
> checks (plus changing the in-code documentation to explicitly say that
> resolve() will always return a valid object and all other cases will be
> reject()ed with an appropriate error code).

Maybe we can add an optional `default` parameter preventing the function to reject and simply resolving with that value.

Comment 11

[Gabriele Svelto [:gsvelto]]

Filed bug 1069808, see if the description there is sensible. The idea of a `default` parameter is also intriguing but I'd rather wait to see if there's actual code which might need it. Let's keep things simple for now.

Comment 12

[Salvador de la Puente González [:salva]]

Thank a lot Gabriel!

Comment 13

[Mathieu Rhéaume]

Comment on attachment 8491072 [details] [review]
Github PR for Bug 1062806 Using LazyLoader and Premises

I have updated the PR with the workaround and put the TODO. 

Would you mind to take a look at the PR ?

If it's ok, I'll mark it as checkin-needed.

I should be able to take care of the bug related to lazy_loader today or tomorrow.

Thanks.

Comment 14

[Salvador de la Puente González [:salva]]

Comment on attachment 8491072 [details] [review]
Github PR for Bug 1062806 Using LazyLoader and Premises

I'm removing the review flag. Please, consider the comment on GitHub and ask again. Thank you.

Comment 15

[Mathieu Rhéaume]

Comment on attachment 8491072 [details] [review]
Github PR for Bug 1062806 Using LazyLoader and Premises

Hi salvador,

I just updated the PR with your comments and removed the else clause.

We had to have the on error func as if lazy_loader returns a reject premise, we can catch it with this func and then do what we used to do in the old xhr error.

Thanks for reviewing.

Comment 16

[Salvador de la Puente González [:salva]]

Comment on attachment 8491072 [details] [review]
Github PR for Bug 1062806 Using LazyLoader and Premises

Address the little nit on GitHub and it's done. Thank you very much.

Comment 17

[Mathieu Rhéaume]

(In reply to Salvador de la Puente González [:salva] from comment #16)
> Comment on attachment 8491072 [details] [review]
> Github PR for Bug 1062806 Using LazyLoader and Premises
> 
> Address the little nit on GitHub and it's done. Thank you very much.

Done. 

Thanks a lot for guiding me through it.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/db907399e143ab079d9a839ad2e402ecc84b73d3

Comment 19

[Marcelino Veiga Tuimil [:sonmarce]]

Closing, as it was already merged in master