Make loot-core compatible with exactOptionalPropertyTypes
#4214
Conversation
actual-github-bot
bot
changed the title
loot-core compatible with exactOptionalPropertyTypesloot-core compatible with exactOptionalPropertyTypes
✅ 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
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were unchanged |
jfdoming
changed the title
loot-core compatible with exactOptionalPropertyTypesloot-core compatible with exactOptionalPropertyTypes
WalkthroughThis pull request introduces a series of type refinements across multiple files in the Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/loot-core/src/server/errors.ts (1)
27-35: LGTM! Enhanced type definition for SyncError meta property.The complex union type with explicit undefined improves type safety while maintaining flexibility for different error scenarios.
Consider adding JSDoc comments to document the different meta shapes and their use cases.
packages/loot-core/src/server/api-models.ts (1)
57-57: LGTM! Consider adding a type assertion for better type safety.The conditional spread operator ensures
group_idis only included whencat_groupexists. This aligns well withexactOptionalPropertyTypes.Consider adding a type assertion to make the relationship between
cat_groupandgroup_idmore explicit:- ...(category.cat_group && { group_id: category.cat_group }), + ...(category.cat_group && { group_id: category.cat_group as string }),packages/loot-core/src/client/actions/budgets.ts (1)
178-178: Update documentation to reflect required parameter.Since
cloudSyncis now a required parameter, the documentation should be updated to reflect this change./** * cloudSync is used to determine if the duplicate budget - * should be synced to the server + * should be synced to the server. + * @required This parameter must be provided. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4214.mdis excluded by!**/*.md
📒 Files selected for processing (13)
packages/loot-core/src/client/accounts/accountsSlice.ts(1 hunks)packages/loot-core/src/client/actions/budgets.ts(2 hunks)packages/loot-core/src/client/data-hooks/reports.ts(1 hunks)packages/loot-core/src/client/data-hooks/transactions.ts(3 hunks)packages/loot-core/src/client/query-hooks.ts(1 hunks)packages/loot-core/src/client/state-types/modals.d.ts(2 hunks)packages/loot-core/src/client/state-types/notifications.d.ts(1 hunks)packages/loot-core/src/mocks/index.ts(3 hunks)packages/loot-core/src/server/api-models.ts(1 hunks)packages/loot-core/src/server/errors.ts(2 hunks)packages/loot-core/src/shared/test-helpers.ts(1 hunks)packages/loot-core/src/shared/transactions.ts(3 hunks)packages/loot-core/src/types/server-handlers.d.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/loot-core/src/client/accounts/accountsSlice.ts
🧰 Additional context used
📓 Learnings (1)
packages/loot-core/src/client/data-hooks/transactions.ts (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/loot-core/src/client/data-hooks/transactions.ts:32-88
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In the React `useTransactions` hook defined in `packages/loot-core/src/client/data-hooks/transactions.ts`, changes to `options` (e.g., `options.pageCount`) should be applied on the next query without adding them to the `useEffect` dependency array, to avoid unnecessary rerenders when options change.
🔇 Additional comments (17)
packages/loot-core/src/client/state-types/notifications.d.ts (2)
4-4: LGTM! Type definitions are now more explicit.The changes correctly make optional properties more explicit by adding
| undefinedto their types, which is necessary for compatibility with TypeScript'sexactOptionalPropertyTypesflag.Also applies to: 7-8, 10-20
5-6: Address the TODO comment about unhandled warning type.The comment indicates that the 'warning' type is unhandled, yet it's included in the type union. This inconsistency should be resolved.
Would you like me to help investigate how warnings are handled in the codebase?
packages/loot-core/src/client/query-hooks.ts (1)
56-56: LGTM! Improved error handling in return type.The change to conditionally spread the error property (
...(error && { error })) is a more precise implementation that aligns with TypeScript'sexactOptionalPropertyTypes. This ensures the error property only exists when there's an actual error.packages/loot-core/src/client/data-hooks/reports.ts (1)
30-32: LGTM! Improved object construction with conditional properties.The changes improve type safety by:
- Only including
conditionsandmetadatawhen they exist- Using nullish coalescing for
conditionsOpto provide a safe defaultpackages/loot-core/src/server/errors.ts (1)
3-3: LGTM! Improved type safety for PostError meta property.The change from optional property to required property with explicit undefined improves type safety and aligns with
exactOptionalPropertyTypes.packages/loot-core/src/shared/test-helpers.ts (1)
22-22: LGTM! Type definition aligns withexactOptionalPropertyTypes.The explicit
| undefinedin the type definition makes it clear thatdatacan be undefined, which is good for type safety.packages/loot-core/src/mocks/index.ts (3)
101-104: LGTM! Early return improves code clarity.The early return when
categoriesis undefined makes the code more maintainable and aligns withexactOptionalPropertyTypes.
126-126: LGTM! Conditional spread operator enhances type safety.Using the spread operator ensures
categoryis only included when it exists, preventing undefined values from being explicitly set.
193-193: LGTM! Conditional amount handling is more explicit.The conditional spread for
amountmakes it clear when the property should be included.packages/loot-core/src/client/actions/budgets.ts (1)
168-169: LGTM! Explicit optional types enhance type safety.The explicit
| undefinedin optional parameter types aligns withexactOptionalPropertyTypes.packages/loot-core/src/client/data-hooks/transactions.ts (2)
93-95: Improve object construction for better type safety.The conditional object construction ensures that
pageCountis only included when defined, aligning with TypeScript'sexactOptionalPropertyTypes.
126-126: Enhance error handling with conditional spreading.The use of conditional spreading for error objects improves type safety by ensuring error properties are only included when they exist. The consolidation of error handling in
usePreviewTransactionsreduces code duplication.Also applies to: 280-280, 284-284
packages/loot-core/src/client/state-types/modals.d.ts (1)
45-45: Improve type definitions for optional properties.The explicit
| undefinedunion type for optional properties aligns with TypeScript'sexactOptionalPropertyTypesflag, making the optionality more precise.Also applies to: 77-77
packages/loot-core/src/shared/transactions.ts (2)
253-258: Improve payee update logic with destructuring.The refactored payee update logic is more explicit and type-safe, correctly handling the case where child transactions inherit the parent's payee.
267-270: Enhance type safety with conditional spreading.The conditional spreading of subtransactions in both
updateTransactionanddeleteTransactionfunctions ensures type safety by only including the property when it exists.Also applies to: 293-296
packages/loot-core/src/types/server-handlers.d.ts (2)
193-193: Improve type definitions for GoCardless handlers.The explicit
| undefinedunion type forupgradingAccountIdin GoCardless handlers aligns with TypeScript'sexactOptionalPropertyTypesflag.Also applies to: 229-229
386-387: Enhance type safety for budget operations.The explicit
| undefinedunion type for budget identifiers indelete-budgetandduplicate-budgethandlers improves type safety and aligns with TypeScript'sexactOptionalPropertyTypesflag.Also applies to: 400-401
| @@ -27,9 +27,9 @@ function toJS(rows: CustomReportData[]) { | |||
| includeCurrentInterval: row.include_current === 1, | |||
| showUncategorized: row.show_uncategorized === 1, | |||
| graphType: row.graph_type, | |||
| conditions: row.conditions, | |||
| ...(row.conditions && { conditions: row.conditions }), | |||
There was a problem hiding this comment.
Nit: Would this also work? This form is more readable IMO.
| ...(row.conditions && { conditions: row.conditions }), | |
| conditions: row.conditions ? row.conditions : undefined, |
There was a problem hiding this comment.
Unfortunately no, this rule makes it so you have to either fully omit the key, or allow the type to be undefined. So in this case, we either do the ugly spread thing, or we update CustomReportEntity to allow conditions to be undefined. Ideally I'd like to avoid making our model objects accept undefined, but defer to you
Part 2 from #4189