Summary

Modernize RB.DiffFragmentQueue and RB.DataUtils.

Review Request #14098 — Created Aug. 9, 2024 and updated 7 months, 3 weeks ago

Information

Owner

david*

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description*

This change converts the old RB.DiffFragmentQueueView to TypeScript,

along with the DataUtils methods which it uses. This involves several

pieces:

Changed the queue to not inherit from a Backbone view. This doesn't

  actually do any view things, it's purely an orchestration class.
Renamed it to DiffFragmentQueue and moved into a utils/ directory.
Updated the DataUtils methods to return promises for their results,

  and marked the callback function parameters as deprecated.
Rewrote the fragment loading to use promises and async/await instead

  of $.funcQueue, and fetch instead of $.ajax.

Testing Done


Ran js-tests.
Opened a review request with a bunch of diff comments and saw all the

  diff comment fragments load correctly.
Opened the review dialog with a bunch of pending diff comments and saw

  all the diff comment fragments load correctly.
Expanded sections of the diff for the reviews and review dialogs.

Commits

Summary	ID
Modernize RB.DiffFragmentQueue and RB.DataUtils. This change converts the old `RB.DiffFragmentQueueView` to TypeScript, along with the DataUtils methods which it uses. This involves several pieces: - Changed the queue to not inherit from a Backbone view. This doesn't actually do any view things, it's purely an orchestration class. - Renamed it to `DiffFragmentQueue` and moved into a utils/ directory. - Updated the `DataUtils` methods to return promises for their results, and marked the callback function parameters as deprecated. - Rewrote the fragment loading to use promises and async/await instead of `$.funcQueue`, and `fetch` instead of `$.ajax`. Testing Done: - Ran js-tests. - Opened a review request with a bunch of diff comments and saw all the diff comment fragments load correctly. - Opened the review dialog with a bunch of pending diff comments and saw all the diff comment fragments load correctly. - Expanded sections of the diff for the reviews and review dialogs.	5f643857a42cb67caff1c32bf3e153accd5935d7

Summary

Modernize RB.DiffFragmentQueue and RB.DataUtils.

This change converts the old `RB.DiffFragmentQueueView` to TypeScript, along with the DataUtils methods which it uses. This involves several pieces: - Changed the queue to not inherit from a Backbone view. This doesn't actually do any view things, it's purely an orchestration class. - Renamed it to `DiffFragmentQueue` and moved into a utils/ directory. - Updated the `DataUtils` methods to return promises for their results, and marked the callback function parameters as deprecated. - Rewrote the fragment loading to use promises and async/await instead of `$.funcQueue`, and `fetch` instead of `$.ajax`. Testing Done: - Ran js-tests. - Opened a review request with a bunch of diff comments and saw all the diff comment fragments load correctly. - Opened the review dialog with a bunch of pending diff comments and saw all the diff comment fragments load correctly. - Expanded sections of the diff for the reviews and review dialogs.

5f643857a42cb67caff1c32bf3e153accd5935d7

Issues

Description	From	Last Updated
Should be , optional.	chipx86	8 months, 3 weeks ago
Should be , optional. These will apply to the others below. Since they're deprecated, we should also add a Deprecated …	chipx86	8 months, 3 weeks ago
Should this just be interface?	chipx86	8 months, 3 weeks ago
For others, we've been putting the optional entries below the required ones.	chipx86	8 months, 3 weeks ago
Should be , optional.	chipx86	8 months, 3 weeks ago
This should use the as T, format, since <T>var is deprecated.	chipx86	8 months, 3 weeks ago
Can be Record<string, QueuedLoad[]>.	chipx86	8 months, 3 weeks ago
Can be Record<string, string>.	chipx86	8 months, 3 weeks ago
I think it'd be nice to define these as interfaces, so the docs can live with them.	chipx86	7 months, 4 weeks ago
Same with this.	chipx86	7 months, 3 weeks ago
Should use the /* ... */ form.	chipx86	8 months, 3 weeks ago
There are no open issues

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/common/utils/dataUtils.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
Should be , optional.
```

reviewboard/static/rb/js/common/utils/dataUtils.ts (Diff revision 1)

The issue has been resolved. Show all issues

Should be , optional.

These will apply to the others below.

Since they're deprecated, we should also add a Deprecated to the param for each.

reviewboard/static/rb/js/common/utils/dataUtils.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
Should this just be interface?
```
reviewboard/static/rb/js/common/utils/dataUtils.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
For others, we've been putting the optional entries below the required ones.
```
reviewboard/static/rb/js/common/utils/dataUtils.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
Should be , optional.
```
reviewboard/static/rb/js/common/utils/dataUtils.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
This should use the as T, format, since <T>var is deprecated.
```
reviewboard/static/rb/js/reviews/utils/diffFragmentQueue.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
Can be Record<string, QueuedLoad[]>.
```
reviewboard/static/rb/js/reviews/utils/diffFragmentQueue.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
Can be Record<string, string>.
```

reviewboard/static/rb/js/reviews/utils/diffFragmentQueue.ts (Diff revision 1)

The issue has been dropped. Show all issues

I think it'd be nice to define these as interfaces, so the docs can live with them.

david 8 months, 3 weeks ago

That seems like a lot for a completely private, internal method. I could add the docs as /** */ comments inside the type structure maybe?

chipx86 7 months, 4 weeks ago

Better to keep them here, then. We won't get autodoc'd stuff out of there if/when we ever turn that on. Don't worry about it.

reviewboard/static/rb/js/reviews/utils/diffFragmentQueue.ts (Diff revision 1)

The issue has been dropped. Show all issues

Same with this.

reviewboard/static/rb/js/reviews/utils/diffFragmentQueue.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
Should use the /* ... */ form.
```

Commits:

	Summary	ID
	Modernize RB.DiffFragmentQueue and RB.DataUtils. This change converts the old `RB.DiffFragmentQueueView` to TypeScript, along with the DataUtils methods which it uses. This involves several pieces: - Changed the queue to not inherit from a Backbone view. This doesn't actually do any view things, it's purely an orchestration class. - Renamed it to `DiffFragmentQueue` and moved into a utils/ directory. - Updated the `DataUtils` methods to return promises for their results, and marked the callback function parameters as deprecated. - Rewrote the fragment loading to use promises and async/await instead of `$.funcQueue`, and `fetch` instead of `$.ajax`. Testing Done: - Ran js-tests. - Opened a review request with a bunch of diff comments and saw all the diff comment fragments load correctly. - Opened the review dialog with a bunch of pending diff comments and saw all the diff comment fragments load correctly. - Expanded sections of the diff for the reviews and review dialogs.	654e7c8a9b1c9d46baa150b74c3c921f28f98912
	Modernize RB.DiffFragmentQueue and RB.DataUtils. This change converts the old `RB.DiffFragmentQueueView` to TypeScript, along with the DataUtils methods which it uses. This involves several pieces: - Changed the queue to not inherit from a Backbone view. This doesn't actually do any view things, it's purely an orchestration class. - Renamed it to `DiffFragmentQueue` and moved into a utils/ directory. - Updated the `DataUtils` methods to return promises for their results, and marked the callback function parameters as deprecated. - Rewrote the fragment loading to use promises and async/await instead of `$.funcQueue`, and `fetch` instead of `$.ajax`. Testing Done: - Ran js-tests. - Opened a review request with a bunch of diff comments and saw all the diff comment fragments load correctly. - Opened the review dialog with a bunch of pending diff comments and saw all the diff comment fragments load correctly. - Expanded sections of the diff for the reviews and review dialogs.	2879bbb3c1234176774d34f01d825d7f62a127f2

Diff:

Revision 2 (+2392 -1810)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/common/index.ts
	reviewboard/static/rb/js/common/utils/dataUtils.ts
	reviewboard/static/rb/js/common/utils/index.ts
	reviewboard/static/rb/js/utils/tests/dataUtilsTests.es6.js
	reviewboard/static/rb/js/common/utils/tests/index.ts
	reviewboard/static/rb/js/reviewRequestPage/views/reviewRequestPageView.es6.js
	reviewboard/static/rb/js/reviewRequestPage/views/tests/baseStatusUpdatesEntryViewTests.es6.js
	10 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Use promises in other parts of the code that use DataUtils, and add suppressions to unit tests so we don't get warnings on the console when testing legacy methods.

Commits:

	Summary	ID
	Modernize RB.DiffFragmentQueue and RB.DataUtils. This change converts the old `RB.DiffFragmentQueueView` to TypeScript, along with the DataUtils methods which it uses. This involves several pieces: - Changed the queue to not inherit from a Backbone view. This doesn't actually do any view things, it's purely an orchestration class. - Renamed it to `DiffFragmentQueue` and moved into a utils/ directory. - Updated the `DataUtils` methods to return promises for their results, and marked the callback function parameters as deprecated. - Rewrote the fragment loading to use promises and async/await instead of `$.funcQueue`, and `fetch` instead of `$.ajax`. Testing Done: - Ran js-tests. - Opened a review request with a bunch of diff comments and saw all the diff comment fragments load correctly. - Opened the review dialog with a bunch of pending diff comments and saw all the diff comment fragments load correctly. - Expanded sections of the diff for the reviews and review dialogs.	2879bbb3c1234176774d34f01d825d7f62a127f2
	Modernize RB.DiffFragmentQueue and RB.DataUtils. This change converts the old `RB.DiffFragmentQueueView` to TypeScript, along with the DataUtils methods which it uses. This involves several pieces: - Changed the queue to not inherit from a Backbone view. This doesn't actually do any view things, it's purely an orchestration class. - Renamed it to `DiffFragmentQueue` and moved into a utils/ directory. - Updated the `DataUtils` methods to return promises for their results, and marked the callback function parameters as deprecated. - Rewrote the fragment loading to use promises and async/await instead of `$.funcQueue`, and `fetch` instead of `$.ajax`. Testing Done: - Ran js-tests. - Opened a review request with a bunch of diff comments and saw all the diff comment fragments load correctly. - Opened the review dialog with a bunch of pending diff comments and saw all the diff comment fragments load correctly. - Expanded sections of the diff for the reviews and review dialogs.	5f643857a42cb67caff1c32bf3e153accd5935d7

Diff:

Revision 3 (+2470 -1826)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/common/index.ts
	reviewboard/static/rb/js/common/utils/dataUtils.ts
	reviewboard/static/rb/js/common/utils/index.ts
	reviewboard/static/rb/js/utils/tests/dataUtilsTests.es6.js
	reviewboard/static/rb/js/common/utils/tests/index.ts
	reviewboard/static/rb/js/reviewRequestPage/models/reviewRequestPageModel.es6.js
	reviewboard/static/rb/js/reviewRequestPage/models/tests/reviewRequestPageModelTests.es6.js
	12 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.