Modernize RB.DiffFragmentQueue and RB.DataUtils.

Review Request #14098 — Created Aug. 9, 2024 and updated

Information

Review Board
master

Reviewers

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.
  • 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.
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
Description From Last Updated

Should be , optional.

chipx86chipx86

Should be , optional. These will apply to the others below. Since they're deprecated, we should also add a Deprecated …

chipx86chipx86

Should this just be interface?

chipx86chipx86

For others, we've been putting the optional entries below the required ones.

chipx86chipx86

Should be , optional.

chipx86chipx86

This should use the as T, format, since <T>var is deprecated.

chipx86chipx86

Can be Record<string, QueuedLoad[]>.

chipx86chipx86

Can be Record<string, string>.

chipx86chipx86

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

chipx86chipx86

Same with this.

chipx86chipx86

Should use the /* ... */ form.

chipx86chipx86
chipx86
  1. 
      
  2. Show all issues

    Should be , optional.

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

  4. Show all issues

    Should this just be interface?

  5. Show all issues

    For others, we've been putting the optional entries below the required ones.

  6. Show all issues

    Should be , optional.

  7. Show all issues

    This should use the as T, format, since <T>var is deprecated.

  8. Show all issues

    Can be Record<string, QueuedLoad[]>.

  9. Show all issues

    Can be Record<string, string>.

  10. reviewboard/static/rb/js/reviews/utils/diffFragmentQueue.ts (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

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

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

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

  11. reviewboard/static/rb/js/reviews/utils/diffFragmentQueue.ts (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Same with this.

  12. Show all issues

    Should use the /* ... */ form.

  13. 
      
david
david
Review request changed
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

Checks run (2 succeeded)

flake8 passed.
JSHint passed.