Summary

Convert BaseCollection.fetch to return a promise.

Review Request #11629 — Created May 27, 2021 and submitted Oct. 3, 2021, 2:08 p.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

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
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.

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.	c6399fe4c61a8bfad98ec8e28a99a93699f16102

Summary

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

Issues

Description	From	Last Updated
Col: 11 Expected ':' and instead saw '_loadBranches'.	reviewbot	May 27, 2021, 9:57 a.m.
Col: 27 Expected '}' to match '{' from line 9 and instead saw '{'.	reviewbot	May 27, 2021, 9:57 a.m.
Col: 13 Expected an assignment or function call and instead saw an expression.	reviewbot	May 27, 2021, 9:57 a.m.
Col: 18 Missing semicolon.	reviewbot	May 27, 2021, 9:57 a.m.
Col: 9 Unrecoverable syntax error. (49% scanned).	reviewbot	May 27, 2021, 9:57 a.m.
Col: 30 Expected ')' and instead saw 'function'.	reviewbot	May 27, 2021, 9:57 a.m.
Col: 40 Missing semicolon.	reviewbot	May 27, 2021, 9:58 a.m.
Col: 17 Expected an assignment or function call and instead saw an expression.	reviewbot	May 27, 2021, 9:58 a.m.
Col: 22 Missing semicolon.	reviewbot	May 27, 2021, 10 a.m.
Col: 30 Expected ')' and instead saw 'function'.	reviewbot	May 27, 2021, 10 a.m.
Col: 40 Missing semicolon.	reviewbot	May 27, 2021, 10 a.m.
Col: 17 Expected an assignment or function call and instead saw an expression.	reviewbot	May 27, 2021, 10 a.m.
Col: 22 Missing semicolon.	reviewbot	May 27, 2021, 10 a.m.
Col: 46 Expected ')' and instead saw 'function'.	reviewbot	May 27, 2021, 10 a.m.
Col: 56 Missing semicolon.	reviewbot	May 27, 2021, 10 a.m.
Col: 17 Expected an assignment or function call and instead saw an expression.	reviewbot	May 27, 2021, 10 a.m.
Col: 22 Missing semicolon.	reviewbot	May 27, 2021, 10 a.m.
Col: 37 Expected ')' and instead saw 'function'.	reviewbot	May 27, 2021, 10 a.m.
Col: 47 Missing semicolon.	reviewbot	May 27, 2021, 10 a.m.
Col: 17 Expected an assignment or function call and instead saw an expression.	reviewbot	May 27, 2021, 10 a.m.
Col: 22 Missing semicolon.	reviewbot	May 27, 2021, 10 a.m.
Col: 56 Expected ')' and instead saw 'function'.	reviewbot	May 27, 2021, 10 a.m.
Col: 66 Missing semicolon.	reviewbot	May 27, 2021, 10 a.m.
Col: 21 Expected an assignment or function call and instead saw an expression.	reviewbot	May 27, 2021, 10 a.m.
Col: 26 Missing semicolon.	reviewbot	May 27, 2021, 10 a.m.
Col: 40 Expected ')' and instead saw 'function'.	reviewbot	May 27, 2021, 10 a.m.
Col: 50 Missing semicolon.	reviewbot	May 27, 2021, 10 a.m.
Col: 17 Expected an assignment or function call and instead saw an expression.	reviewbot	May 27, 2021, 10 a.m.
Col: 22 Missing semicolon.	reviewbot	May 27, 2021, 10 a.m.
Col: 47 Expected ')' and instead saw 'function'.	reviewbot	May 27, 2021, 10 a.m.
Col: 10 'promiseToCallbacks' is defined but never used.	reviewbot	June 7, 2021, 10:24 p.m.
Col: 29 Missing semicolon.	reviewbot	June 7, 2021, 10:25 p.m.
Col: 65 Missing semicolon.	reviewbot	June 7, 2021, 10:25 p.m.
Col: 50 Missing semicolon.	reviewbot	June 7, 2021, 10:28 p.m.
Col: 50 Missing semicolon.	reviewbot	June 7, 2021, 10:31 p.m.
Col: 2 Unnecessary semicolon.	reviewbot	June 7, 2021, 10:31 p.m.
Can we put this in a Version Changed block, like: Version Changed: 5.0: ... We should document every public function …	chipx86	Sept. 9, 2021, 11:55 a.m.
Might be worth documenting why we have a function for this instead of just referencing _loadBranches directly (and same below). …	chipx86	Sept. 9, 2021, noon
Can we add a Clicked suffix to these? That'd be more in line with the naming for most other click …	chipx86	Sept. 9, 2021, noon
So this pattern doesn't quite preserve the logic we had before, and it might bite us. I think we need …	chipx86	Sept. 9, 2021, 12:03 p.m.
This returns a jqXHR (jQuery specialization of XMLHttpRequest), not a promise. That seems an important distinction in return types. I'm …	chipx86	Sept. 9, 2021, 10:28 p.m.
This is missing a return type.	chipx86	Sept. 9, 2021, 10:27 p.m.
Hmm, the semantics of the function definitely changed here. We should document that. Do we depend on the old return …	chipx86	Sept. 9, 2021, 10:28 p.m.
This is missing Args. Can we also add a Version Added?	chipx86	Sept. 9, 2021, 12:01 p.m.
Why the move to : function(?	chipx86	Sept. 18, 2021, 1:43 p.m.
Col: 31 Expected an identifier and instead saw '.'.	reviewbot	Sept. 9, 2021, 10:34 p.m.
Col: 32 Expected ':' and instead saw '$el'.	reviewbot	Sept. 9, 2021, 10:34 p.m.
Col: 36 Expected an operator and instead saw '.'.	reviewbot	Sept. 9, 2021, 10:34 p.m.
Col: 35 Expected an identifier and instead saw '?'.	reviewbot	Sept. 9, 2021, 10:34 p.m.
Col: 36 Expected an assignment or function call and instead saw an expression.	reviewbot	Sept. 9, 2021, 10:34 p.m.
Col: 37 Missing semicolon.	reviewbot	Sept. 9, 2021, 10:34 p.m.
Col: 27 Expected an identifier and instead saw '.'.	reviewbot	Sept. 9, 2021, 10:34 p.m.
Col: 28 Expected ':' and instead saw '$el'.	reviewbot	Sept. 9, 2021, 10:34 p.m.
Col: 31 Expected an identifier and instead saw '?'.	reviewbot	Sept. 9, 2021, 10:34 p.m.
Col: 32 Expected an operator and instead saw '.'.	reviewbot	Sept. 9, 2021, 10:34 p.m.
Col: 32 Expected an assignment or function call and instead saw an expression.	reviewbot	Sept. 9, 2021, 10:34 p.m.
Col: 33 Missing semicolon.	reviewbot	Sept. 9, 2021, 10:34 p.m.
We're no longer calling checkFetchNext(), which was part of the bug fix for the commit list not loading new commits. …	chipx86	Sept. 18, 2021, 1:45 p.m.
You'll actually need a new Version Changed block for each version. The parser doesn't know how to split this up …	chipx86	Sept. 18, 2021, 1:32 p.m.
There's 2 spaces between await and this.fetch.	chipx86	Sept. 18, 2021, 1:33 p.m.
This will have to be above Args to render correctly. It's considered paragraph-level content for the description.	chipx86	Sept. 18, 2021, 1:33 p.m.
Should this be fetch(...) or fetch: function(...)?	chipx86	Sept. 18, 2021, 1:33 p.m.
We should probably say in all of these that callbacks are now deprecated and will be removed in Review Board …	chipx86	Sept. 18, 2021, 1:34 p.m.
Entirely a food-for-thought, and not necessary for this change. What about opting into decorators support in Babel? (It's currently a …	chipx86	Sept. 18, 2021, 1:43 p.m.
Do we need to still set this._fetchURL?	chipx86	Sept. 18, 2021, 1:35 p.m.
Leftover debugging?	chipx86	Sept. 18, 2021, 1:35 p.m.
The "5.0" has to be on its own line, or it won't be interpreted as a section, just text.	chipx86	Sept. 18, 2021, 1:36 p.m.
Same here.	chipx86	Sept. 18, 2021, 1:36 p.m.
Mind using parens to make the grouping more clear?	chipx86	Oct. 3, 2021, 12:04 a.m.
Col: 2 Missing semicolon.	reviewbot	Sept. 9, 2021, 10:34 p.m.
Col: 31 Missing semicolon.	reviewbot	Sept. 9, 2021, 10:34 p.m.
Col: 32 Expected ':' and instead saw '$el'.	reviewbot	Sept. 18, 2021, 3:39 p.m.
Col: 31 Expected an identifier and instead saw '.'.	reviewbot	Sept. 18, 2021, 3:39 p.m.
Col: 35 Expected an identifier and instead saw '?'.	reviewbot	Sept. 18, 2021, 3:40 p.m.
Col: 36 Expected an operator and instead saw '.'.	reviewbot	Sept. 18, 2021, 3:40 p.m.
Col: 36 Expected an assignment or function call and instead saw an expression.	reviewbot	Sept. 18, 2021, 3:40 p.m.
Col: 37 Missing semicolon.	reviewbot	Sept. 18, 2021, 3:40 p.m.
Col: 27 Expected an identifier and instead saw '.'.	reviewbot	Sept. 18, 2021, 3:40 p.m.
Col: 28 Expected ':' and instead saw '$el'.	reviewbot	Sept. 18, 2021, 3:40 p.m.
Col: 31 Expected an identifier and instead saw '?'.	reviewbot	Sept. 18, 2021, 3:40 p.m.
Col: 32 Expected an operator and instead saw '.'.	reviewbot	Sept. 18, 2021, 3:40 p.m.
Col: 32 Expected an assignment or function call and instead saw an expression.	reviewbot	Sept. 18, 2021, 3:40 p.m.
Col: 33 Missing semicolon.	reviewbot	Sept. 18, 2021, 3:40 p.m.

flake8 passed.

JSHint failed.

JSHint

```
Warning: Showing 30 of 80 failures.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 1)
Show all issues
```
Col: 11
 Expected ':' and instead saw '_loadBranches'.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 1)
Show all issues
```
Col: 27
 Expected '}' to match '{' from line 9 and instead saw '{'.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 1)
Show all issues
```
Col: 13
 Expected an assignment or function call and instead saw an expression.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 1)
Show all issues
```
Col: 18
 Missing semicolon.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 1)
Show all issues
```
Col: 9
 Unrecoverable syntax error. (49% scanned).
```
reviewboard/static/rb/js/newReviewRequest/views/tests/postCommitViewTests.es6.js (Diff revision 1)
Show all issues
```
Col: 30
 Expected ')' and instead saw 'function'.
```
reviewboard/static/rb/js/newReviewRequest/views/tests/postCommitViewTests.es6.js (Diff revision 1)
Show all issues
```
Col: 40
 Missing semicolon.
```
reviewboard/static/rb/js/newReviewRequest/views/tests/postCommitViewTests.es6.js (Diff revision 1)
Show all issues
```
Col: 17
 Expected an assignment or function call and instead saw an expression.
```
reviewboard/static/rb/js/newReviewRequest/views/tests/postCommitViewTests.es6.js (Diff revision 1)
Show all issues
```
Col: 22
 Missing semicolon.
```
reviewboard/static/rb/js/newReviewRequest/views/tests/postCommitViewTests.es6.js (Diff revision 1)
Show all issues
```
Col: 30
 Expected ')' and instead saw 'function'.
```
reviewboard/static/rb/js/newReviewRequest/views/tests/postCommitViewTests.es6.js (Diff revision 1)
Show all issues
```
Col: 40
 Missing semicolon.
```
reviewboard/static/rb/js/newReviewRequest/views/tests/postCommitViewTests.es6.js (Diff revision 1)
Show all issues
```
Col: 17
 Expected an assignment or function call and instead saw an expression.
```
reviewboard/static/rb/js/newReviewRequest/views/tests/postCommitViewTests.es6.js (Diff revision 1)
Show all issues
```
Col: 22
 Missing semicolon.
```
reviewboard/static/rb/js/resources/collections/tests/resourceCollectionTests.es6.js (Diff revision 1)
Show all issues
```
Col: 46
 Expected ')' and instead saw 'function'.
```
reviewboard/static/rb/js/resources/collections/tests/resourceCollectionTests.es6.js (Diff revision 1)
Show all issues
```
Col: 56
 Missing semicolon.
```
reviewboard/static/rb/js/resources/collections/tests/resourceCollectionTests.es6.js (Diff revision 1)
Show all issues
```
Col: 17
 Expected an assignment or function call and instead saw an expression.
```
reviewboard/static/rb/js/resources/collections/tests/resourceCollectionTests.es6.js (Diff revision 1)
Show all issues
```
Col: 22
 Missing semicolon.
```
reviewboard/static/rb/js/resources/collections/tests/resourceCollectionTests.es6.js (Diff revision 1)
Show all issues
```
Col: 37
 Expected ')' and instead saw 'function'.
```
reviewboard/static/rb/js/resources/collections/tests/resourceCollectionTests.es6.js (Diff revision 1)
Show all issues
```
Col: 47
 Missing semicolon.
```
reviewboard/static/rb/js/resources/collections/tests/resourceCollectionTests.es6.js (Diff revision 1)
Show all issues
```
Col: 17
 Expected an assignment or function call and instead saw an expression.
```
reviewboard/static/rb/js/resources/collections/tests/resourceCollectionTests.es6.js (Diff revision 1)
Show all issues
```
Col: 22
 Missing semicolon.
```
reviewboard/static/rb/js/resources/collections/tests/resourceCollectionTests.es6.js (Diff revision 1)
Show all issues
```
Col: 56
 Expected ')' and instead saw 'function'.
```
reviewboard/static/rb/js/resources/collections/tests/resourceCollectionTests.es6.js (Diff revision 1)
Show all issues
```
Col: 66
 Missing semicolon.
```
reviewboard/static/rb/js/resources/collections/tests/resourceCollectionTests.es6.js (Diff revision 1)
Show all issues
```
Col: 21
 Expected an assignment or function call and instead saw an expression.
```
reviewboard/static/rb/js/resources/collections/tests/resourceCollectionTests.es6.js (Diff revision 1)
Show all issues
```
Col: 26
 Missing semicolon.
```
reviewboard/static/rb/js/resources/collections/tests/resourceCollectionTests.es6.js (Diff revision 1)
Show all issues
```
Col: 40
 Expected ')' and instead saw 'function'.
```
reviewboard/static/rb/js/resources/collections/tests/resourceCollectionTests.es6.js (Diff revision 1)
Show all issues
```
Col: 50
 Missing semicolon.
```
reviewboard/static/rb/js/resources/collections/tests/resourceCollectionTests.es6.js (Diff revision 1)
Show all issues
```
Col: 17
 Expected an assignment or function call and instead saw an expression.
```
reviewboard/static/rb/js/resources/collections/tests/resourceCollectionTests.es6.js (Diff revision 1)
Show all issues
```
Col: 22
 Missing semicolon.
```
reviewboard/static/rb/js/resources/collections/tests/resourceCollectionTests.es6.js (Diff revision 1)
Show all issues
```
Col: 47
 Expected ')' and instead saw 'function'.
```

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

Diff:

Revision 2 (+1550 -760)

Show changes

	.browserslistrc
	reviewboard/static/rb/js/collections/baseCollection.es6.js
	reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js
	reviewboard/static/rb/js/newReviewRequest/views/tests/postCommitViewTests.es6.js
	reviewboard/static/rb/js/resources/collections/repositoryCommitsCollection.es6.js
	reviewboard/static/rb/js/resources/collections/resourceCollection.es6.js
	reviewboard/static/rb/js/resources/collections/tests/resourceCollectionTests.es6.js
	reviewboard/static/rb/js/utils/apiUtils.es6.js
	2 more

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/utils/apiUtils.es6.js (Diff revision 2)
Show all issues
```
Col: 10
 'promiseToCallbacks' is defined but never used.
```
reviewboard/static/rb/js/utils/apiUtils.es6.js (Diff revision 2)
Show all issues
```
Col: 29
 Missing semicolon.
```
reviewboard/static/rb/js/utils/apiUtils.es6.js (Diff revision 2)
Show all issues
```
Col: 65
 Missing semicolon.
```

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

Diff:

Revision 3 (+1548 -760)

Show changes

	.browserslistrc
	reviewboard/static/rb/js/collections/baseCollection.es6.js
	reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js
	reviewboard/static/rb/js/newReviewRequest/views/tests/postCommitViewTests.es6.js
	reviewboard/static/rb/js/resources/collections/repositoryCommitsCollection.es6.js
	reviewboard/static/rb/js/resources/collections/resourceCollection.es6.js
	reviewboard/static/rb/js/resources/collections/tests/resourceCollectionTests.es6.js
	reviewboard/static/rb/js/utils/apiUtils.es6.js
	2 more

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/utils/apiUtils.es6.js (Diff revision 3)
Show all issues
```
Col: 50
 Missing semicolon.
```

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

Diff:

Revision 4 (+1548 -760)

Show changes

	.browserslistrc
	reviewboard/static/rb/js/collections/baseCollection.es6.js
	reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js
	reviewboard/static/rb/js/newReviewRequest/views/tests/postCommitViewTests.es6.js
	reviewboard/static/rb/js/resources/collections/repositoryCommitsCollection.es6.js
	reviewboard/static/rb/js/resources/collections/resourceCollection.es6.js
	reviewboard/static/rb/js/resources/collections/tests/resourceCollectionTests.es6.js
	reviewboard/static/rb/js/utils/apiUtils.es6.js
	2 more

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/utils/apiUtils.es6.js (Diff revision 4)
Show all issues
```
Col: 50
 Missing semicolon.
```
reviewboard/static/rb/js/utils/apiUtils.es6.js (Diff revision 4)
Show all issues
```
Col: 2
 Unnecessary 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.	1596d1d77ef38df834e0326d5471a6e34d53c10c
	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

Diff:

Revision 5 (+1548 -760)

Show changes

	.browserslistrc
	reviewboard/static/rb/js/collections/baseCollection.es6.js
	reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js
	reviewboard/static/rb/js/newReviewRequest/views/tests/postCommitViewTests.es6.js
	reviewboard/static/rb/js/resources/collections/repositoryCommitsCollection.es6.js
	reviewboard/static/rb/js/resources/collections/resourceCollection.es6.js
	reviewboard/static/rb/js/resources/collections/tests/resourceCollectionTests.es6.js
	reviewboard/static/rb/js/utils/apiUtils.es6.js
	2 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/collections/baseCollection.es6.js (Diff revision 5)

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.

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

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.

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

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.

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.

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

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.

david Sept. 9, 2021, 10:28 p.m.

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.

reviewboard/static/rb/js/resources/collections/resourceCollection.es6.js (Diff revision 5)
Show all issues
```
This is missing a return type.
```

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

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?

david Sept. 9, 2021, 10:28 p.m.

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.

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?

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

Diff:

Revision 6 (+1700 -764)

Show changes

	.browserslistrc
	reviewboard/static/rb/js/collections/baseCollection.es6.js
	reviewboard/static/rb/js/newReviewRequest/views/commitsView.es6.js
	reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js
	reviewboard/static/rb/js/newReviewRequest/views/tests/postCommitViewTests.es6.js
	reviewboard/static/rb/js/resources/collections/repositoryCommitsCollection.es6.js
	reviewboard/static/rb/js/resources/collections/resourceCollection.es6.js
	reviewboard/static/rb/js/resources/collections/tests/resourceCollectionTests.es6.js
	3 more

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 6)
Show all issues
```
Col: 31
 Expected an identifier and instead saw '.'.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 6)
Show all issues
```
Col: 32
 Expected ':' and instead saw '$el'.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 6)
Show all issues
```
Col: 36
 Expected an operator and instead saw '.'.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 6)
Show all issues
```
Col: 35
 Expected an identifier and instead saw '?'.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 6)
Show all issues
```
Col: 36
 Expected an assignment or function call and instead saw an expression.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 6)
Show all issues
```
Col: 37
 Missing semicolon.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 6)
Show all issues
```
Col: 27
 Expected an identifier and instead saw '.'.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 6)
Show all issues
```
Col: 28
 Expected ':' and instead saw '$el'.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 6)
Show all issues
```
Col: 31
 Expected an identifier and instead saw '?'.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 6)
Show all issues
```
Col: 32
 Expected an operator and instead saw '.'.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 6)
Show all issues
```
Col: 32
 Expected an assignment or function call and instead saw an expression.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 6)
Show all issues
```
Col: 33
 Missing semicolon.
```
reviewboard/static/rb/js/utils/apiUtils.es6.js (Diff revision 6)
Show all issues
```
Col: 2
 Missing semicolon.
```
reviewboard/static/rb/js/views/tests/reviewDialogViewTests.es6.js (Diff revision 6)
Show all issues
```
Col: 31
 Missing semicolon.
```

```
I love where this is all going.
```

reviewboard/static/rb/js/collections/baseCollection.es6.js (Diff revision 6)

Show all issues

Why the move to : function(?

david Sept. 18, 2021, 1:55 p.m.

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

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

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.

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

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.

reviewboard/static/rb/js/resources/collections/repositoryCommitsCollection.es6.js (Diff revision 6)
Show all issues
```
There's 2 spaces between await and this.fetch.
```

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

Show all issues

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

reviewboard/static/rb/js/resources/collections/resourceCollection.es6.js (Diff revision 6)
Show all issues
```
Should this be fetch(...) or fetch: function(...)?
```
1. david Sept. 18, 2021, 1:55 p.m.
  See above.

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

Show all issues

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

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.

david Sept. 18, 2021, 1:55 p.m.

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

reviewboard/static/rb/js/resources/collections/resourceCollection.es6.js (Diff revision 6)
Show all issues
```
Do we need to still set this._fetchURL?
```
1. david Sept. 18, 2021, 1:55 p.m.
  fetchNext already did that, so this was redundant.
reviewboard/static/rb/js/resources/collections/tests/resourceCollectionTests.es6.js (Diff revision 6)
Show all issues
```
Leftover debugging?
```

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

Show all issues

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

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

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

Show all issues

Mind using parens to make the grouping more clear?

david Sept. 18, 2021, 1:55 p.m.

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.

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

Diff:

Revision 7 (+1698 -764)

Show changes

	.browserslistrc
	reviewboard/static/rb/js/.jshintrc
	reviewboard/static/rb/js/collections/baseCollection.es6.js
	reviewboard/static/rb/js/newReviewRequest/views/commitsView.es6.js
	reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js
	reviewboard/static/rb/js/newReviewRequest/views/tests/postCommitViewTests.es6.js
	reviewboard/static/rb/js/resources/collections/repositoryCommitsCollection.es6.js
	reviewboard/static/rb/js/resources/collections/resourceCollection.es6.js
	4 more

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 7)
Show all issues
```
Col: 32
 Expected ':' and instead saw '$el'.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 7)
Show all issues
```
Col: 31
 Expected an identifier and instead saw '.'.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 7)
Show all issues
```
Col: 35
 Expected an identifier and instead saw '?'.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 7)
Show all issues
```
Col: 36
 Expected an operator and instead saw '.'.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 7)
Show all issues
```
Col: 36
 Expected an assignment or function call and instead saw an expression.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 7)
Show all issues
```
Col: 37
 Missing semicolon.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 7)
Show all issues
```
Col: 27
 Expected an identifier and instead saw '.'.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 7)
Show all issues
```
Col: 28
 Expected ':' and instead saw '$el'.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 7)
Show all issues
```
Col: 31
 Expected an identifier and instead saw '?'.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 7)
Show all issues
```
Col: 32
 Expected an operator and instead saw '.'.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 7)
Show all issues
```
Col: 32
 Expected an assignment or function call and instead saw an expression.
```
reviewboard/static/rb/js/newReviewRequest/views/postCommitView.es6.js (Diff revision 7)
Show all issues
```
Col: 33
 Missing semicolon.
```

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to master (fed25b6)