Add first pass at response typings #563
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #563 +/- ##
=======================================
Coverage 81.72% 81.72%
=======================================
Files 6 6
Lines 279 279
Branches 43 43
=======================================
Hits 228 228
Misses 36 36
Partials 15 15
Continue to review full report at Codecov.
|
| (options?: MethodArguments & AuxiliaryArguments): Promise<ResponseType>; | ||
| ( | ||
| options: MethodArguments & AuxiliaryArguments, | ||
| callback: WebAPIResultCallback, |
There was a problem hiding this comment.
maybe WebAPIResultCallback needs a generic type constraint of ResponseType?
| @@ -0,0 +1,2430 @@ | |||
| export interface Im { | |||
There was a problem hiding this comment.
could some of these interfaces be unified with interfaces defined in src/methods.ts? i know that these define the API outputs, and those the API inputs. unifying them could come with the potential win that similar structures only need to be updated in one place. keeping them separate comes with the win of decoupling. just food for thought at this point.
|
whoa, thanks for this contribution! i'm so glad you're willing to help further the goals of #509 (and more general #496) i think this approach is very nice. i like that method types and response types don't directly depend on one another, and that the WebClient actually pairs them up. you said this output was generated, would you mind sharing the script? i think it would be useful to keep in the repo to "sync" with the OpenAPI spec every once in a while. was there much manual manipulation of the output after the script was run? i'm really interested in understanding the process. reviewing a change this big will definitely take some time. i'd like to build the package and try using it locally to see what the typescript language service has to offer with all these awesome types to work with! i don't expect to have time to do this in the next week or so. i encourage other community members to check out this patch and leave some feedback. since i see this as a change that might take some iteration before landing, and since potentially more people may like to get involved, perhaps it makes sense to retarget this PR onto a new feature branch (e.g. i saw that you tried to undo the style changes introduced by prettier, thanks. the majority of the style changes i'd probably want to revert before this lands on master. some were great though, and i think we can contribute some changes to our |
|
@clavin i'd love to hear your thoughts, if you have a chance. |
|
@neekolas Hi. If you do not have a problem, could you please rebase it ? 👍 refs: neekolas#2 |
Summary
Adds auto-generated response types for all API methods. Typings are generated based on the public JSON schema, so it is only as complete as that schema is. Addresses #509. Does not make any adjustments to runtime code.
Not entirely sure how strict you want to be on the cases where there is incomplete data. Master currently shows a typescript error any time you try and access an API response property directly, which I took as a hint and erred on the side of strictness. I cannot find any cases where Typescript throws an error or warning on my branch that doesn't also get a warning or error in master.
It wouldn't let me commit without resolving a few unrelated tslint errors. I went through your .tslint.json and you guys have
no-redundant-jsdocturned on - so I think I was fixing a lint error and not creating one - but it's entirely possible there's something mismatched in my local environment to yours that is triggering it for me.Requirements (place an
xin each[ ])