• 
      

    Improve a lot of typing for BaseResource.

    Review Request #13847 — Created May 8, 2024 and submitted

    Information

    Review Board
    release-7.x

    Reviewers

    The BaseResource implementation in TypeScript had (and continues to
    have) a number of issues. A lot of this stems from just bad design
    decisions that were made before we even thought too much about typing or
    polymorphism, but some of it is just mistakes or poor understanding of
    TypeScript when I first converted it.

    This change does the following:

    1. Switches the order of TOptions and TExtraOptions. I had followed
      what Backbone.Model did, but didn't realize that the spina
      BaseModel swaps them. This was making it so our call to
      CombinedModelConstructorOptions was creating errors.
    2. Adds a base resource data interface, and extends that for subclasses
      that currently define the resource data structure.
    3. Moved payloadFileKeys into the BaseResource implementation. This
      was currently only defined in FileAttachment, but it's used inside
      of BaseResource and by other (not-yet-typescriptified) subclasses.
    4. Changed validate to take in a Partial<TDefaults> instead of
      TDefaults, which matches our usage of this method.
    5. Added type labels for everything that's in prototypeAttrs as
      instance members too. This shuts up the type checker about these
      magical properties.
    6. Properly passed in the attributes type for DraftReview.
    7. Added some missing type labels.

    Changes 1 & 2 are technically breaking, but the only class that was
    affected was ReviewRequest, and it seems highly unlikely that any
    third-party code is using this yet, so I want to do it now.

    What this doesn't fix is our decision to override fetch, save, and
    destroy with incompatible signatures. I'm still puzzling over what to
    do there to make it so we don't get huge horrible warnings about any
    resource class not being compatible with Backbone.Model.

    Ran js-tests.

    Summary ID
    Improve a lot of typing for BaseResource.
    The BaseResource implementation in TypeScript had (and continues to have) a number of issues. A lot of this stems from just bad design decisions that were made before we even thought too much about typing or polymorphism, but some of it is just mistakes or poor understanding of TypeScript when I first converted it. This change does the following: 1. Switches the order of `TOptions` and `TExtraOptions`. I had followed what `Backbone.Model` did, but didn't realize that the spina `BaseModel` swaps them. This was making it so our call to `CombinedModelConstructorOptions` was creating errors. 2. Adds a base resource data interface, and extends that for subclasses that currently define the resource data structure. 3. Moved `payloadFileKeys` into the BaseResource implementation. This was currently only defined in FileAttachment, but it's used inside of BaseResource and by other (not-yet-typescriptified) subclasses. 4. Changed `validate` to take in a `Partial<TDefaults>` instead of `TDefaults`, which matches our usage of this method. 5. Added type labels for everything that's in `prototypeAttrs` as instance members too. This shuts up the type checker about these magical properties. 6. Properly passed in the attributes type for DraftReview. 7. Added some missing type labels. Changes 1 & 2 are technically breaking, but the only class that was affected was `ReviewRequest`, and it seems highly unlikely that any third-party code is using this yet, so I want to do it now. What this doesn't fix is our decision to override `fetch`, `save`, and `destroy` with incompatible signatures. I'm still puzzling over what to do there to make it so we don't get huge horrible warnings about any resource class not being compatible with `Backbone.Model`. Testing Done: Ran js-tests.
    27d0a29426f903447b7e0cc7279d252a82d3cad9
    Description From Last Updated

    Missing Version Added.

    chipx86chipx86

    Can we maybe pull the _.zip(...) out into a variable, to make this read a bit better?

    chipx86chipx86
    chipx86
    1. 
        
    2. Show all issues

      Missing Version Added.

    3. Show all issues

      Can we maybe pull the _.zip(...) out into a variable, to make this read a bit better?

    4. 
        
    david
    david
    maubin
    1. Ship It!
    2. 
        
    chipx86
    1. Ship It!
    2. 
        
    david
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-7.x (86409cc)