• 
      

    Convert apiUtils to TypeScript.

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

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

    This is all alphabetical but the last two items.

    chipx86chipx86

    Same here.

    chipx86chipx86

    Can you use :js:func: for these?

    chipx86chipx86

    Can we pull the <div> out of these? They don't need to be in the translated strings.

    chipx86chipx86
    chipx86
    1. 
        
    2. Show all issues

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

      1. Where do you see that? It's not in the list at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Lexical_grammar#reserved_words

      2. 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
    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. 
        
    david
    chipx86
    1. A few small nits, but everything overall looks great.

    2. Show all issues

      This is all alphabetical but the last two items.

    3. Show all issues

      Same here.

    4. Show all issues

      Can you use :js:func: for these?

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

      Can we pull the <div> out of these? They don't need to be in the translated strings.

    6. 
        
    david
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to master (8711766)