Support type-checking on spreadsheet fields (part 1) #3093
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
jfdoming
force-pushed
the
jfdoming/strict-binding-types-part-1
branch
from
5b1ccd9
to
4831db2
Compare
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| SheetName extends SheetNames = any, | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| FieldName extends SheetFields<SheetName> = any, |
There was a problem hiding this comment.
See PR description; the approach is to use any for a slow rollout and revert the use of any after the last PR in the stack is merged (3 PRs, can all be merged around the same time).
| @@ -722,7 +745,7 @@ export function SheetCell({ | |||
| } | |||
| textAlign={textAlign} | |||
| {...props} | |||
| value={sheetValue} | |||
| value={(sheetValue ?? '').toString()} | |||
There was a problem hiding this comment.
Would this work here?
| value={(sheetValue ?? '').toString()} | |
| value={String(sheetValue)} |
There was a problem hiding this comment.
I think for null this would return 'null' instead of '', I think we probably want '' here?
This PR (along with the subsequent ones, see follow-up PRs for parts 2 and 3) adds stricter types to the "spreadsheet" utilities used in the app. The ultimate goal is to convert
src/components/accounts/Account.jsxto Typescript (see previous attempt here along with comments). In terms of types being added, the approach taken is to add types field-by-field, and require field names to be present in a type map (see theSpreadsheetFieldTypestype added in this PR).Since this is a large change, the approach taken for rollout is to add generic types in this PR, initially default them to
any, and finally remove the defaults in the last PR. Please see this link for a collapsed version of all the PRs.Feedback on the approach (rollout or typing) welcome!