Improve a lot of typing for BaseResource.

 






 

 


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

 




















 

  

   Information

  





 
 

  david
 









 
 

  Review Board
 









 
 

  release-7.x
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
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. 

  2. Adds a base resource data interface, and extends that for subclasses
    
   that currently define the resource data structure.
    3. 

  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. 

  4. Changed validate to take in a Partial<TDefaults> instead of
    
TDefaults, which matches our usage of this method.
    5. 

  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. 

  6. Properly passed in the attributes type for DraftReview.
    7. 

  7. Added some missing type labels.
    8. 



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




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Missing Version Added.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



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

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  4. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  

 
















 



 

  

   




david




   

   

  

 



 















 



 

  

   




david




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  








 
Change Summary:

 
A couple small fixes





 



  

   
Commits:

   
    





 

  
   


    

    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.

     


    		
    60c50e7fbc2407b31c820d2f5d20ca3986b90d74

   




   


    

    

     

      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

   



  
 







   		
  




  

   
Diff:

   
    
 Revision 3 (+188 -66)
Show changes
 
    reviewboard/static/rb/js/common/resources/models/baseCommentModel.ts 
    reviewboard/static/rb/js/common/resources/models/baseResourceModel.ts 
    reviewboard/static/rb/js/common/resources/models/draftReviewModel.ts 
    reviewboard/static/rb/js/common/resources/models/fileAttachmentModel.ts 
    reviewboard/static/rb/js/common/resources/models/reviewModel.ts 
    reviewboard/static/rb/js/common/resources/models/reviewReplyModel.ts 
    reviewboard/static/rb/js/common/resources/models/reviewRequestModel.ts 
 

   		
  



 









 

  
Checks run (2 succeeded)



  

 flake8
 passed.
 





  

 JSHint
 passed.
 





 















  

 
















 





 

  

   




maubin




   

   







  

 



 

  


   

   

    
 
maubin


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 









     

    

   

  

  

   
   Loading...