Convert apiUtils to TypeScript.

Review Request #14157 — Created Sept. 11, 2024 and updated

Information

Review Board
master

Reviewers

This change converts our API methods to TypeScript. Our apiUtils file
has a few methods, primary among them being apiCall. This change adds
typing for everything, and cleans up some old cruft. It does not yet
make any changes to the internals of how API requests work.

In order to facilitate using jasmine for unit tests. these methods are
all exported under a new top-level object called API.

  • Ran js-tests.
  • Smoke tested the UI and verified that things still worked correctly.
Summary ID
Convert apiUtils to TypeScript.
This change converts our API methods to TypeScript. Our apiUtils file has a few methods, primary among them being `apiCall`. This change adds typing for everything, and cleans up some old cruft. It does not yet make any changes to the internals of how API requests work. In order to facilitate using jasmine for unit tests. these methods are all exported under a new top-level object called `API`. Testing Done: - Ran js-tests. - Smoke tested the UI and verified that things still worked correctly.
a657224ac9ce148f81fce048e852172f42ff500f
Description From Last Updated

I don't see where we provide compatibility stubs for the old function names in the RB namespace.

chipx86chipx86

call is a reserved function name. We'll need to call this something else.

chipx86chipx86

I think we want to swap these, given alphabetizing of module names (resourceCollection.ts has it that way).

chipx86chipx86

This can be Record<string, unknown> I think.

chipx86chipx86

While we're changing this, we really should use _.template and gain smart text escaping, since we failed at that originally.

chipx86chipx86

Can you swap these? Alphabetical order.

chipx86chipx86

These should be swapped.

chipx86chipx86

Can this fit on one line now?

chipx86chipx86

Description needs to be on its own paragraph.

chipx86chipx86

Do we want to make this a literal of methods?

chipx86chipx86

Can we use :js:func for these?

chipx86chipx86

We'll need to massage this into one line.

chipx86chipx86

Second sentence needs to be its own paragraph, like in Python.

chipx86chipx86

This would probably be better as paint at this point. That said, there's one quirk with how the nodes combine. …

chipx86chipx86

While here, we should add role="button".

chipx86chipx86

These should use :js:func:.

chipx86chipx86

Can we use our standard multi-arg function syntax for these arguments?

chipx86chipx86

Should probably have Version Added here.

chipx86chipx86
chipx86
  1. 
      
  2. Show all issues

    call is a reserved function name. We'll need to call this something else.

    1. Maybe reserved is the wrong word, but it has special meaning for function invocation (functionName.call(this, ...)), and feels really confusing in that way. I don't think it's a great name to use for a function, as API.call then looks like a standard function .call() syntax on API, rather than being our own API call function.

      Maybe we should just call this request or something, if we're modernizing API invocation calls anyway?

  3. 
      
david
chipx86
  1. 
      
  2. Show all issues

    I don't see where we provide compatibility stubs for the old function names in the RB namespace.

    1. That happens in reviewboard/static/rb/js/common/utils/index.ts

  3. Show all issues

    I think we want to swap these, given alphabetizing of module names (resourceCollection.ts has it that way).

  4. Show all issues

    This can be Record<string, unknown> I think.

  5. reviewboard/static/rb/js/common/utils/apiUtils.ts (Diff revision 2)
     
     
     
     
     
     
    Show all issues

    While we're changing this, we really should use _.template and gain smart text escaping, since we failed at that originally.

  6. reviewboard/static/rb/js/common/utils/apiUtils.ts (Diff revision 2)
     
     
     
     
     
     
    Show all issues

    Can you swap these? Alphabetical order.

  7. 
      
david
Review request changed
Commits:
Summary ID
Convert apiUtils to TypeScript.
This change converts our API methods to TypeScript. Our apiUtils file has a few methods, primary among them being `apiCall`. This change adds typing for everything, and cleans up some old cruft. It does not yet make any changes to the internals of how API requests work. In order to facilitate using jasmine for unit tests. these methods are all exported under a new top-level object called `API`. Testing Done: - Ran js-tests. - Smoke tested the UI and verified that things still worked correctly.
201e9d99dcada845bdde4bc279628f00f18b1eb9
Convert apiUtils to TypeScript.
This change converts our API methods to TypeScript. Our apiUtils file has a few methods, primary among them being `apiCall`. This change adds typing for everything, and cleans up some old cruft. It does not yet make any changes to the internals of how API requests work. In order to facilitate using jasmine for unit tests. these methods are all exported under a new top-level object called `API`. Testing Done: - Ran js-tests. - Smoke tested the UI and verified that things still worked correctly.
a657224ac9ce148f81fce048e852172f42ff500f
Diff:

Revision 3 (+872 -452)

Show changes

reviewboard/staticbundles.py
reviewboard/static/rb/js/accountPrefsPage/views/oauthApplicationsView.ts
reviewboard/static/rb/js/accountPrefsPage/views/oauthTokensView.ts
reviewboard/static/rb/js/common/collections/baseCollection.ts
reviewboard/static/rb/js/common/resources/collections/repositoryCommitsCollection.ts
reviewboard/static/rb/js/common/resources/collections/resourceCollection.ts
reviewboard/static/rb/js/common/resources/models/baseResourceModel.ts
reviewboard/static/rb/js/common/resources/models/draftResourceChildModelMixin.ts
24 more

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
maubin
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. Show all issues

    These should be swapped.

  3. Show all issues

    Can this fit on one line now?

  4. Show all issues

    Description needs to be on its own paragraph.

  5. reviewboard/static/rb/js/common/utils/apiUtils.ts (Diff revision 3)
     
     
     
     
     
    Show all issues

    Do we want to make this a literal of methods?

  6. Show all issues

    Can we use :js:func for these?

  7. Show all issues

    We'll need to massage this into one line.

  8. Show all issues

    Second sentence needs to be its own paragraph, like in Python.

  9. reviewboard/static/rb/js/common/utils/apiUtils.ts (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    This would probably be better as paint at this point.

    That said, there's one quirk with how the nodes combine. To get a space between an element and text, you'll need to do:

    paint<...>`
        <p>
         <b>${_`Blah ....`}</b>
         ${' '}
         ${status}
        </p>
        ...
    `
    
  10. reviewboard/static/rb/js/common/utils/apiUtils.ts (Diff revision 3)
     
     
     
     
     
     
     
     
     
    Show all issues

    While here, we should add role="button".

  11. Show all issues

    These should use :js:func:.

  12. Show all issues

    Can we use our standard multi-arg function syntax for these arguments?

  13. Show all issues

    Should probably have Version Added here.

  14. 
      
Loading...