Convert BaseCollection.fetch to return a promise.

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

Information

Review Board
master

Reviewers

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 ID
Convert BaseCollection.fetch to return a promise.
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. Testing Done: - Ran js-tests. - Manually tested data population on the review dialog and post commit views.
c6399fe4c61a8bfad98ec8e28a99a93699f16102
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: 36 Expected an operator and instead saw '.'.

reviewbotreviewbot

Col: 35 Expected an identifier 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: 32 Expected ':' and instead saw '$el'.

reviewbotreviewbot

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

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 ID
Convert BaseCollection.fetch to return a promise.
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`. 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. Testing Done: - Ran js-tests. - Manually tested data population on the review dialog and post commit views.
1ea75eb10311e7e215d92b73039b86b05799f344
Convert BaseCollection.fetch to return a promise.
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`. 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. Testing Done: - Ran js-tests. - Manually tested data population on the review dialog and post commit views.
cf1142ac68ffc3d3850d5bdc828e8c2c247eb01a

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

david
Review request changed
Change Summary:

Namespace helper method, fix Review Bot errors.

Commits:
Summary ID
Convert BaseCollection.fetch to return a promise.
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`. 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. Testing Done: - Ran js-tests. - Manually tested data population on the review dialog and post commit views.
cf1142ac68ffc3d3850d5bdc828e8c2c247eb01a
Convert BaseCollection.fetch to return a promise.
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`. 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. Testing Done: - Ran js-tests. - Manually tested data population on the review dialog and post commit views.
2a7be8ea22bc567b890bdbeb139d3ffcf1a60e85

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

david
Review request changed
Change Summary:

Semicolon.

Commits:
Summary ID
Convert BaseCollection.fetch to return a promise.
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`. 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. Testing Done: - Ran js-tests. - Manually tested data population on the review dialog and post commit views.
2a7be8ea22bc567b890bdbeb139d3ffcf1a60e85
Convert BaseCollection.fetch to return a promise.
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`. 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. Testing Done: - Ran js-tests. - Manually tested data population on the review dialog and post commit views.
1596d1d77ef38df834e0326d5471a6e34d53c10c

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

david
chipx86
  1. 
      
  2. Show all issues

    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. Show all issues

    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. Show all issues

    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)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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. Show all issues

    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. Show all issues

    This is missing a return type.

  8. Show all issues

    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)
     
     
     
     
     
     
     
     
    Show all issues

    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 ID
Convert BaseCollection.fetch to return a promise.
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`. 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. Testing Done: - Ran js-tests. - Manually tested data population on the review dialog and post commit views.
79ee4f343b971904505ac601393a356b1e016301
Convert BaseCollection.fetch to return a promise.
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. Testing Done: - Ran js-tests. - Manually tested data population on the review dialog and post commit views.
cbad9df88c7ebf1844dd5fb79a3b05dad9127ffd

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

chipx86
  1. I love where this is all going.

  2. Show all issues

    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. Show all issues

    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. Show all issues

    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. Show all issues

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

  6. Show all issues

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

  7. Show all issues

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

  8. Show all issues

    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)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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. Show all issues

    Do we need to still set this._fetchURL?

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

  11. Show all issues

    Leftover debugging?

  12. Show all issues

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

  13. Show all issues

    Same here.

  14. reviewboard/static/rb/js/utils/apiUtils.es6.js (Diff revision 6)
     
     
     
     
    Show all issues

    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.

  15. 
      
david
Review request changed
Commits:
Summary ID
Convert BaseCollection.fetch to return a promise.
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. Testing Done: - Ran js-tests. - Manually tested data population on the review dialog and post commit views.
cbad9df88c7ebf1844dd5fb79a3b05dad9127ffd
Convert BaseCollection.fetch to return a promise.
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. Testing Done: - Ran js-tests. - Manually tested data population on the review dialog and post commit views.
c6399fe4c61a8bfad98ec8e28a99a93699f16102

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

chipx86
  1. Ship It!
  2. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to master (fed25b6)