-
Notifications
You must be signed in to change notification settings - Fork 343
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate toolkit reports to TypeScript #3367
Migrate toolkit reports to TypeScript #3367
Conversation
interface YNABSharedLibImpl { | ||
dateFormatter: YNABDateFormatter; | ||
defaultInstance: YNABSharedLibInstance; | ||
} | ||
|
||
type ExtendWithAny<T extends {}> = { [key: string]: any } & T; | ||
|
||
type YNABSharedLib = ExtendWithAny<YNABSharedLibImpl>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can avoid this renaming of the root interface by adding this extends
clause.
interface YNABSharedLibImpl { | |
dateFormatter: YNABDateFormatter; | |
defaultInstance: YNABSharedLibInstance; | |
} | |
type ExtendWithAny<T extends {}> = { [key: string]: any } & T; | |
type YNABSharedLib = ExtendWithAny<YNABSharedLibImpl>; | |
interface YNABSharedLib extends Record<string, any> { | |
dateFormatter: YNABDateFormatter; | |
defaultInstance: YNABSharedLibInstance; | |
} |
Separately, I would recommend against using any
here, why do you think it is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you think it is needed
YNABSharedLib has a lot of stuff inside of it and typing it properly will take a lot of time. As it was out of scope of current PR, but I still needed a way for TS to stop yelling at me for accessing unknown properties I come up with such solution. But I'll your suggested code more, I'll update it
innerSize: '50%', | ||
}, | ||
} as Highcharts.SeriesOptionsRegistry['SeriesPieOptions' | 'SeriesColumnOptions'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest moving this to the declaration for chart
since it will be less brittle to changes in Highcharts
, and keep us from accidentally casting to a wrong type.
const chart: Highcharts.SeriesOptionsRegistry['SeriesPieOptions' | 'SeriesColumnOptions'] = { ... };
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow what you mean here. Chart is instance of Highcharts.Chart
and here we're casting only one of it's options object
src/extension/features/toolkit-reports/pages/net-worth/components/legend/component.tsx
Outdated
Show resolved
Hide resolved
src/extension/features/toolkit-reports/pages/net-worth/component.tsx
Outdated
Show resolved
Hide resolved
src/extension/features/toolkit-reports/pages/inflow-outflow/component.tsx
Outdated
Show resolved
Hide resolved
...expense/components/monthly-transaction-totals-table/components/collapsable-row/component.tsx
Outdated
Show resolved
Hide resolved
...it-reports/pages/income-vs-expense/components/monthly-transaction-totals-table/component.tsx
Outdated
Show resolved
Hide resolved
I only reviewed about 30% of the diff because it's quite large. A few things I noticed are:
|
Thanks @jakehamtexas ! Your comment were really helpful 🙂
Sorry, but I don't get why we need it? (and why d.ts in particular) |
thanks for doing this! great stuff |
GitHub Issue (if applicable): n/a
Explanation of Bugfix/Feature/Modification:
I migrated toolkit reports to TypeScript. In most reports this meant just adding types, but some of them was incompatible with types (mostly due to using
Map
as structures with pre-defined set of keys instead of using plain objects). For those I had to modify code a bit, but I tested and there shouldn't be any change in behavior.Please note this PR will conflict with #3360 so I suggest it will be merged first and then I'll resolve conflicts in this PR