Convert BaseCollection.fetch to return a promise.

Review Request #11629 — Created May 27, 2021 and updated

david
Review Board
master
reviewboard

This is the first of what is likely to be many steps to move our
codebase towards a more modern feel of using promises and
async/await for asynchronous operations. In this change,
BaseCollection.fetch has been changed, along with everything that
actually uses that including similar methods inside
ResourceCollection. The old callback usage is still available through
a helper which will wrap the returned promise, but causes a warning to
be logged to the console.

This also tweaks to our browserslist config in order to ensure that
async/await won't get polyfilled out due to niche browsers
(primarily on mobile) that we really don't care about supporting.

  • Ran js-tests.
  • Manually tested data population on the review dialog and post commit
    views.
Summary
Convert BaseCollection.fetch to return a promise.
Description From Last Updated

Col: 11 Expected ':' and instead saw '_loadBranches'.

reviewbotreviewbot

Col: 27 Expected '}' to match '{' from line 9 and instead saw '{'.

reviewbotreviewbot

Col: 13 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 18 Missing semicolon.

reviewbotreviewbot

Col: 9 Unrecoverable syntax error. (49% scanned).

reviewbotreviewbot

Col: 30 Expected ')' and instead saw 'function'.

reviewbotreviewbot

Col: 40 Missing semicolon.

reviewbotreviewbot

Col: 17 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 22 Missing semicolon.

reviewbotreviewbot

Col: 30 Expected ')' and instead saw 'function'.

reviewbotreviewbot

Col: 40 Missing semicolon.

reviewbotreviewbot

Col: 17 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 22 Missing semicolon.

reviewbotreviewbot

Col: 46 Expected ')' and instead saw 'function'.

reviewbotreviewbot

Col: 56 Missing semicolon.

reviewbotreviewbot

Col: 17 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 22 Missing semicolon.

reviewbotreviewbot

Col: 37 Expected ')' and instead saw 'function'.

reviewbotreviewbot

Col: 47 Missing semicolon.

reviewbotreviewbot

Col: 17 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 22 Missing semicolon.

reviewbotreviewbot

Col: 56 Expected ')' and instead saw 'function'.

reviewbotreviewbot

Col: 66 Missing semicolon.

reviewbotreviewbot

Col: 21 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 26 Missing semicolon.

reviewbotreviewbot

Col: 40 Expected ')' and instead saw 'function'.

reviewbotreviewbot

Col: 50 Missing semicolon.

reviewbotreviewbot

Col: 17 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 22 Missing semicolon.

reviewbotreviewbot

Col: 47 Expected ')' and instead saw 'function'.

reviewbotreviewbot

Col: 10 'promiseToCallbacks' is defined but never used.

reviewbotreviewbot

Col: 29 Missing semicolon.

reviewbotreviewbot

Col: 65 Missing semicolon.

reviewbotreviewbot

Col: 50 Missing semicolon.

reviewbotreviewbot

Col: 50 Missing semicolon.

reviewbotreviewbot

Col: 2 Unnecessary semicolon.

reviewbotreviewbot

Can we put this in a Version Changed block, like: Version Changed: 5.0: ... We should document every public function ...

chipx86chipx86

Might be worth documenting why we have a function for this instead of just referencing _loadBranches directly (and same below). ...

chipx86chipx86

Can we add a Clicked suffix to these? That'd be more in line with the naming for most other click ...

chipx86chipx86

So this pattern doesn't quite preserve the logic we had before, and it might bite us. I think we need ...

chipx86chipx86

This returns a jqXHR (jQuery specialization of XMLHttpRequest), not a promise. That seems an important distinction in return types. I'm ...

chipx86chipx86

This is missing a return type.

chipx86chipx86

Hmm, the semantics of the function definitely changed here. We should document that. Do we depend on the old return ...

chipx86chipx86

This is missing Args. Can we also add a Version Added?

chipx86chipx86

Why the move to : function(?

chipx86chipx86

Col: 31 Expected an identifier and instead saw '.'.

reviewbotreviewbot

Col: 32 Expected ':' and instead saw '$el'.

reviewbotreviewbot

Col: 35 Expected an identifier and instead saw '?'.

reviewbotreviewbot

Col: 36 Expected an operator and instead saw '.'.

reviewbotreviewbot

Col: 36 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 37 Missing semicolon.

reviewbotreviewbot

Col: 27 Expected an identifier and instead saw '.'.

reviewbotreviewbot

Col: 28 Expected ':' and instead saw '$el'.

reviewbotreviewbot

Col: 31 Expected an identifier and instead saw '?'.

reviewbotreviewbot

Col: 32 Expected an operator and instead saw '.'.

reviewbotreviewbot

Col: 32 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 33 Missing semicolon.

reviewbotreviewbot

We're no longer calling checkFetchNext(), which was part of the bug fix for the commit list not loading new commits. ...

chipx86chipx86

You'll actually need a new Version Changed block for each version. The parser doesn't know how to split this up ...

chipx86chipx86

There's 2 spaces between await and this.fetch.

chipx86chipx86

This will have to be above Args to render correctly. It's considered paragraph-level content for the description.

chipx86chipx86

Should this be fetch(...) or fetch: function(...)?

chipx86chipx86

We should probably say in all of these that callbacks are now deprecated and will be removed in Review Board ...

chipx86chipx86

Entirely a food-for-thought, and not necessary for this change. What about opting into decorators support in Babel? (It's currently a ...

chipx86chipx86

Do we need to still set this._fetchURL?

chipx86chipx86

Leftover debugging?

chipx86chipx86

The "5.0" has to be on its own line, or it won't be interpreted as a section, just text.

chipx86chipx86

Same here.

chipx86chipx86

Mind using parens to make the grouping more clear?

chipx86chipx86

Col: 2 Missing semicolon.

reviewbotreviewbot

Col: 31 Missing semicolon.

reviewbotreviewbot

Col: 31 Expected an identifier and instead saw '.'.

reviewbotreviewbot

Col: 32 Expected ':' and instead saw '$el'.

reviewbotreviewbot

Col: 35 Expected an identifier and instead saw '?'.

reviewbotreviewbot

Col: 36 Expected an operator and instead saw '.'.

reviewbotreviewbot

Col: 36 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 37 Missing semicolon.

reviewbotreviewbot

Col: 27 Expected an identifier and instead saw '.'.

reviewbotreviewbot

Col: 28 Expected ':' and instead saw '$el'.

reviewbotreviewbot

Col: 31 Expected an identifier and instead saw '?'.

reviewbotreviewbot

Col: 32 Expected an operator and instead saw '.'.

reviewbotreviewbot

Col: 32 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 33 Missing semicolon.

reviewbotreviewbot
Checks run (1 failed, 1 succeeded)
flake8 passed.
JSHint failed.

JSHint

david
Review request changed

Description:

   

This is the first of what is likely to be many steps to move our

    codebase towards a more modern feel of using promises (and eventually
    async) for asynchronous operations. In this change,
    BaseCollection.fetch has been changed, along with everything that
    actually uses that including similar methods inside
~   ResourceCollection.

  ~ ResourceCollection. The old callback usage is still available through
  + a helper which will wrap the returned promise, but causes a warning to
  + be logged to the console.

   
   

This also makes a slight tweak to our browserslist config in order to

    ensure that async won't get polyfilled out due to older chrome on
    android.

Commits:

Summary
-
Convert BaseCollection.fetch to return a promise.
+
Convert BaseCollection.fetch to return a promise.

Diff:

Revision 2 (+1550 -760)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

david
Review request changed
david
Review request changed
david
chipx86
  1. 
      
  2. Can we put this in a Version Changed block, like:

    Version Changed:
        5.0:
        ...
    

    We should document every public function that's been changed to return a Promise. That'll help anyone that's trying to update code in 5.0 or even 6.0 timeframe.

  3. Might be worth documenting why we have a function for this instead of just referencing _loadBranches directly (and same below). I know the reasons now, but I'm sure I'll forget in another year.

  4. Can we add a Clicked suffix to these? That'd be more in line with the naming for most other click handlers in the codebase.

  5. reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    So this pattern doesn't quite preserve the logic we had before, and it might bite us. I think we need to take a harder look at each of these conversions.

    Before, the error handling code would run only if fetch() failed, but now, it runs if fetch() fails or if any of the previously-success code fails.

    Any code called after that await that raises an exception (unlikely in this particular case, but conceivable) would trigger the catch(error) {...}, changing the UI in a way we don't expect.

    An argument could be made that, hey, in this case, we're just going through and setting up an error display, but 1) we'd lose out on the traceback, and 2) that code expects err to be an object with a message.

    I think what we're going to want to do is something more like:

    try {
        await branches.fetch();
    } catch (err) {
        ...
        return;
    }
    
    // success code here
    

    This comment pertains to all other cases like this.

  6. This returns a jqXHR (jQuery specialization of XMLHttpRequest), not a promise. That seems an important distinction in return types.

    I'm a bit concerned by just how much we're changing return types for this function, and how mixed it is. The old version would return a true or false to indicate that the fetch had something to return in some cases, but a jqXHR in this case. The new version replaces the booleans with promises but keeps the jqXHR, but some code calling this assumes it's a promise.

    So we need to fix that, and we have to be careful, because this is public API.

    I don't know the solution. The callbacks situation seems better, but return types are harder. Maybe the the way we're handling callbacks isn't enough? Maybe we need to put the function into a legacy mode if it's taking legacy arguments, and have that also include the return type as part of that legacy mode (in other words, ditch returning promises altogether if receiving callbacks)?

    I don't know. I'm too tired to think of a solution. Got a couple more comments I already wrote up for this file, but since we need to solve this, I'll skip doing a full review for now.

    1. This isn't actually the case. RB.BaseCollection.prototype.fetch() didn't have any return value before, so the fact that we were returning it here also had no return value.

      The old version of fetchPrev/fetchNext supposedly returned true/false, but the only thing that used it was a couple of the resource collection test cases. In fact, the commits subclass overrode it but did not provide the return value either.

  7. This is missing a return type.

  8. Hmm, the semantics of the function definitely changed here. We should document that. Do we depend on the old return value anywhere in the codebase?

    1. As mentioned above, only a couple test cases used it, and not in a way which actually mattered since the rest of the test was much more substantive.

  9. reviewboard/static/rb/js/utils/apiUtils.es6.js (Diff revision 5)
     
     
     
     
     
     
     
     

    This is missing Args.

    Can we also add a Version Added?

  10. 
      
david
Review request changed

Description:

   

This is the first of what is likely to be many steps to move our

~   codebase towards a more modern feel of using promises (and eventually
~   async) for asynchronous operations. In this change,
  ~ codebase towards a more modern feel of using promises and
  ~ async/await for asynchronous operations. In this change,
    BaseCollection.fetch has been changed, along with everything that
    actually uses that including similar methods inside
    ResourceCollection. The old callback usage is still available through
    a helper which will wrap the returned promise, but causes a warning to
    be logged to the console.

   
~  

This also makes a slight tweak to our browserslist config in order to

~   ensure that async won't get polyfilled out due to older chrome on
~   android.

  ~

This also tweaks to our browserslist config in order to ensure that

  ~ async/await won't get polyfilled out due to niche browsers
  ~ (primarily on mobile) that we really don't care about supporting.

Commits:

Summary
-
Convert BaseCollection.fetch to return a promise.
+
Convert BaseCollection.fetch to return a promise.

Diff:

Revision 6 (+1700 -764)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

chipx86
  1. I love where this is all going.

  2. Why the move to : function(?

    1. TBH it's because vim syntax highlighting gets super confused otherwise. Probably as a result of fetch becoming a built-in method.

  3. We're no longer calling checkFetchNext(), which was part of the bug fix for the commit list not loading new commits. Probably a merge issue.

  4. You'll actually need a new Version Changed block for each version. The parser doesn't know how to split this up into individual blocks.

  5. There's 2 spaces between await and this.fetch.

  6. This will have to be above Args to render correctly. It's considered paragraph-level content for the description.

  7. Should this be fetch(...) or fetch: function(...)?

  8. We should probably say in all of these that callbacks are now deprecated and will be removed in Review Board 6.

  9. reviewboard/static/rb/js/resources/collections/resourceCollection.es6.js (Diff revision 6)
     
     
     
     
     
     
     
     
     
     

    Entirely a food-for-thought, and not necessary for this change. What about opting into decorators support in Babel? (It's currently a JS proposal.) Then we could wrap the option checks, the warning, and the promiseToCallbacks call, without repeating this block throughout the codebase.

    1. Hmm, that's a cool idea. I'll play with that for a separate change.

  10. Do we need to still set this._fetchURL?

    1. fetchNext already did that, so this was redundant.

  11. The "5.0" has to be on its own line, or it won't be interpreted as a section, just text.

  12. reviewboard/static/rb/js/utils/apiUtils.es6.js (Diff revision 6)
     
     
     
     

    Mind using parens to make the grouping more clear?

    1. I'm not really sure how it's unclear -- it's just a fallback. It probably would have been fine to just use || but this seemed more correct.

  13. 
      
david
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

Loading...