Fix top-level types of send function
#4145
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
jfdoming
force-pushed
the
jfdoming/fix-send-types
branch
5 times, most recently
from
7f182ef to
224063c
Compare
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
changed the title
send functionsend function
jfdoming
force-pushed
the
jfdoming/fix-send-types
branch
from
77ecf93 to
84fd2b8
Compare
| args, | ||
| { catchErrors = false } = {}, | ||
| ) { | ||
| export const send = function (name, args, { catchErrors = false } = {}) { |
There was a problem hiding this comment.
Removed the T.Send because it was erroring on the return value (currently Promise<unknown> which doesn't match). To fix, we'd have to duplicate the types into this file. Happy to do so if the reviewer thinks it's worth it!
actual-github-bot
bot
changed the title
send functionsend function
WalkthroughThe pull request involves modifications to type handling and function signatures across multiple files in the project. In the Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (2)
✨ 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: 1
🧹 Nitpick comments (2)
packages/loot-core/src/platform/client/fetch/index.web.ts (2)
81-81: Consider reintroducing type annotations for 'send' to maintain type safety.The explicit type annotation
: T.Sendhas been removed from thesendfunction. While this resolves the immediate type error, it reduces type safety and may lead to unintended issues. Consider updating the return type to align with the new function signature and reintroduce the type annotation to maintain clarity and type safety.If you need assistance in adjusting the type definitions to reintroduce the type annotation without duplication, I'm happy to help.
106-106: Consider reintroducing type annotations for 'sendCatch' to maintain type safety.The explicit type annotation
: T.SendCatchhas been removed from thesendCatchfunction. To ensure consistent type safety and clarity, consider updating the type definitions and reapplying the type annotation.If you would like help in updating the type definitions to reintroduce the type annotation, please let me know.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4145.mdis excluded by!**/*.md
📒 Files selected for processing (3)
packages/desktop-client/src/components/modals/EditAccess.tsx(1 hunks)packages/loot-core/src/platform/client/fetch/index.d.ts(1 hunks)packages/loot-core/src/platform/client/fetch/index.web.ts(2 hunks)
🔇 Additional comments (3)
packages/loot-core/src/platform/client/fetch/index.d.ts (3)
23-23: Confirm optional parameters in 'send' function overload.In the second overload of the
sendfunction, confirm that the optional parameters are correctly defined to match the updated function signatures and that it handles cases whereargsoroptionsmight be undefined.
10-17:⚠️ Potential issueUpdate function calls: 'args' parameter is now required in 'send' function.
The
argsparameter in thesendfunction has been changed from optional to required. This modification may cause existing calls tosendwithout theargsargument to fail. Please ensure all calls tosendinclude theargsparameter.Here is a script to identify all
sendcalls missing theargsparameter:
29-34:⚠️ Potential issueUpdate function calls: 'args' parameter is now required in 'sendCatch' function.
The
argsparameter in thesendCatchfunction has been changed from optional to required. Existing calls tosendCatchwithout theargsargument may fail. Please ensure all calls tosendCatchinclude theargsparameter.Here is a script to identify all
sendCatchcalls missing theargsparameter:
| send('access-get-available-users', defaultUserAccess.fileId).then(data => { | ||
| if ('error' in data) { | ||
| setSetError(data.error); | ||
| } else { | ||
| setAvailableUsers( | ||
| data.map(user => [ | ||
| user.userId, | ||
| user.displayName | ||
| ? `${user.displayName} (${user.userName})` | ||
| : user.userName, | ||
| ]), | ||
| ); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Update error handling to match the updated 'send' function behavior.
With the changes to the send function, errors are now thrown instead of being returned when catchErrors is not set to true. The current code checks for an error property in the response, which may no longer be valid. To maintain the existing error handling, consider using sendCatch or passing { catchErrors: true } as an options parameter.
Apply one of the following solutions to update the code:
Option 1: Use sendCatch function
useEffect(() => {
- send('access-get-available-users', defaultUserAccess.fileId).then(data => {
+ sendCatch('access-get-available-users', defaultUserAccess.fileId).then(response => {
if (response.error) {
setSetError(response.error);
} else {
setAvailableUsers(
- data.map(user => [
+ response.data.map(user => [
user.userId,
user.displayName
? `${user.displayName} (${user.userName})`
: user.userName,
]),
);
}
});Option 2: Use send with catchErrors: true option
useEffect(() => {
- send('access-get-available-users', defaultUserAccess.fileId).then(data => {
+ send('access-get-available-users', defaultUserAccess.fileId, { catchErrors: true }).then(response => {
if (response.error) {
setSetError(response.error);
} else {
setAvailableUsers(
- data.map(user => [
+ response.data.map(user => [
user.userId,
user.displayName
? `${user.displayName} (${user.userName})`
: user.userName,
]),
);
}
});Committable suggestion skipped: line range outside the PR's diff.
jfdoming
changed the title
send functionsend function
| | { data: Handlers[K] } | ||
| | { error: { type: 'APIError' | 'InternalError'; message: string } } | ||
| ): Promise< | ||
| | { data: Awaited<ReturnType<Handlers[K]>>; error: undefined } |
There was a problem hiding this comment.
| | { data: Awaited<ReturnType<Handlers[K]>>; error: undefined } | |
| | { data: Awaited<ReturnType<Handlers[K]>> } |
There was a problem hiding this comment.
If I remove this I get Property 'error' does not exist on type in a bunch of places like this:
const { data, error } = await sendCatch('gocardless-get-banks', country);I don't see any other way to keep the ability to destructure like this, any ideas?
There was a problem hiding this comment.
I see. In that case we should probably just merge those two properties and make them both optional. WDYT?
There was a problem hiding this comment.
Hmm, I kind of like being able to assert that data is undefined if error is not, and vice-versa. I think it would make the code a bit messy otherwise unfortunately as you'd have to check both
| | { | ||
| data: undefined; | ||
| error: { type: 'APIError' | 'InternalError'; message: string }; | ||
| } |
There was a problem hiding this comment.
| | { | |
| data: undefined; | |
| error: { type: 'APIError' | 'InternalError'; message: string }; | |
| } | |
| | { | |
| error: { type: 'APIError' | 'InternalError'; message: string }; | |
| } |
| | { data: Handlers[K] } | ||
| | { error: { type: 'APIError' | 'InternalError'; message: string } } | ||
| ): Promise< | ||
| | { data: Awaited<ReturnType<Handlers[K]>>; error: undefined } |
There was a problem hiding this comment.
I see. In that case we should probably just merge those two properties and make them both optional. WDYT?
| | { data: Awaited<ReturnType<Handlers[K]>>; error: undefined } | ||
| | { | ||
| data: undefined; | ||
| error: { type: 'APIError' | 'InternalError'; message: string }; |
There was a problem hiding this comment.
| | { data: Awaited<ReturnType<Handlers[K]>>; error: undefined } | |
| | { | |
| data: undefined; | |
| error: { type: 'APIError' | 'InternalError'; message: string }; | |
| { data?: Awaited<ReturnType<Handlers[K]>> | undefined; error?: { type: 'APIError' | 'InternalError'; message: string } | undefined; } |
* 🐛 Fix Initializing the connection to the local database hang (actualbudget#4375) * fix initializing to the local db * release notes * Add percentage adjustments to schedule templates (actualbudget#4098) (actualbudget#4257) * add percentage adjustments to schedule templates * update release note * remove unecessary parentheses * Update packages/loot-core/src/server/budget/goalsSchedule.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * PR comments addressed * Linting fixes * Updated error handling, added tests * Linting fixes --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: youngcw <calebyoung94@gmail.com> * Custom mapping and import settings for bank sync providers (actualbudget#4253) * barebones UI * add saving and prefs * add last sync functionality * use mapping for synced transactions * note * jest -u * Update VRT * Coderabbit Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * add new fields * rename migration, newer in master * lint * coderabbit * update snapshots * GoCardless handlers fallback and notes * expose new fields from SimpleFIN * update tests * update instructions on GoCardless handlers * lint * feedback * Update VRT --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: youngcw <calebyoung94@gmail.com> * Add fallback value for payeename: 'undefined' - CBC Bank (actualbudget#4384) * Add fallback value for payename: 'undefined' * docs: add release note * Add fallback value for payename: 'undefined' (for negative amounts) * [TypeScript] Convert test page models to TS (actualbudget#4218) * Dummy commit * Delete js snapshots * Move extended expect and test to fixtures * Fix wrong commit * Update VRT * Dummy commit to run GH actions * Convert test page models to TS * Release notes * Fix typecheck errors * New page models to TS * Fix typecheck error * Fix page name * Put awaits on getTableTotals --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * rename two migrations to realign with convention (actualbudget#4343) * Updating sync server package name to @actual-app/sync-server (actualbudget#4370) * updating sync server to have a consistent package name * release notes * Add today button on mobile budget page (actualbudget#4380) * feat: today button on mobile budget page Jumps to the current month * add release note * cleaner onCurrentMonth * Update VRT * use SvgCalendar from icons/v2 Co-authored-by: Matt Fiddaman <github@m.fiddaman.uk> * Update VRT --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Matt Fiddaman <github@m.fiddaman.uk> * Development mode for sync server (React Fast Refresh on port 5006) (actualbudget#4372) * devmode for sync server * removed pluggy from this version * md * code review * changed how open browser * missed this * linter * trigger actions * 🐛 Fix: Error rate limit at user directory page (actualbudget#4397) * Error rate limit * md * 🐛 Fix new proxy middleware dependency missing on prod build (actualbudget#4400) * fix new proxy middleware not installed on prod build * release notes * Remove deprecated imports for several components (actualbudget#4385) * don't unbudget goals * lint * Fixes actualbudget#4069 : Ignore CSV inOutMode during OFX imports (actualbudget#4382) * Ignore inOutMode during OFX imports * Add release notes --------- Co-authored-by: youngcw <calebyoung94@gmail.com> * fix tooltip translation (actualbudget#4402) * [TypeScript] Make `db.runQuery` generic to make it easy to type DB query results (actualbudget#4247) * Make runQuery generic to make it easy to type DB query results. * Release notes * typo * update mapping data for existing synced transactions and always show direction dropdown (actualbudget#4403) * update sync mapping data for existing transactions on sync * show payment direction dropdown regardless of sample data * note * ignore changes in raw_synced_data * Fix top-level types of `send` function (actualbudget#4145) * Add release notes * Fix types of `send` function * Fix `send` types in a number of places * PR feedback * Foundations for the budget automations UI (actualbudget#4308) * Foundations for the budget automations UI * Coderabbit * Fix react-hooks/exhaustive-deps error on useSelected.tsx (actualbudget#4258) * Fix react-hooks/exhaustive-deps error on useSelected.tsx * Release notes * Fix react-hooks/exhaustive-deps error on usePayees.ts (actualbudget#4260) * Fix react-hooks/exhaustive-deps error on usePayees.ts * Rename * Release notes * Fix react-hooks/exhaustive-deps error on useAccounts.ts (actualbudget#4262) * Fix react-hooks/exhaustive-deps error on useAccounts.ts * Release notes * Fix react-hooks/exhaustive-deps error on Titlebar.tsx (actualbudget#4273) * Fix react-hooks/exhaustive-deps error on Titlebar.tsx * Release notes * [WIP] BANKS_WITH_LIMITED_HISTORY constant update (actualbudget#4388) * Fix react-hooks/exhaustive-deps error on useProperFocus.tsx (actualbudget#4259) * Fix react-hooks/exhaustive-deps error on useProperFocus.tsx * Remove comment in eslint config * Release notes * Fix react-hooks/exhaustive-deps error on TransactionsTable.jsx (actualbudget#4268) * Fix react-hooks/exhaustive-deps error on TransactionsTable.jsx * Release notes * Fix lint * Fix react-hooks/exhaustive-deps error on table.tsx (actualbudget#4274) * Fix react-hooks/exhaustive-deps error on table.tsx * Release notes * Fix react-hooks/exhaustive-deps error on useCategories.ts (actualbudget#4261) * Fix react-hooks/exhaustive-deps error on useCategories.ts * Release notes * 👷 Typescript: Improving typing of asyncStorage (global-store.json) (actualbudget#4369) * typing globalprefs * adding release notes * unneeded partial * removing prop that crept in * 📚 Translation batch #1 (actualbudget#4408) * Translation batch * md * Update packages/desktop-client/src/components/settings/Export.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix code review from coderabbit * code review --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Fix currencyToAmount incorrectly converting input (actualbudget#4383) * fix: ensure currencyToAmount works regardless of the configured number format * chore: linting * docs: add release note * test: ensure correct amount is entered for debit when adding split transactions * chore: rename variable thousandsSep to thousandsSeparator Co-authored-by: Joel Jeremy Marquez <joeljeremy.marquez@gmail.com> * chore: rename variable decimalSep to decimalSeparator Co-authored-by: Joel Jeremy Marquez <joeljeremy.marquez@gmail.com> * chore: rename decimalSep and thousandsSep variables to decimalSeparator and thousandsSeparator --------- Co-authored-by: Joel Jeremy Marquez <joeljeremy.marquez@gmail.com> * useDisplayPayee hook to unify payee names in mobile and desktop. (actualbudget#4213) * useDisplayPayee hook to unify payee logic in mobile and desktop * Release notes * Fix typecheck errors * Fix test * Update test * Revert (No payee) color * Fix tests * VRT * Fix category transactions * Fix lint and VRT * Update VRT * Translate --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * Extract transaction related server handlers from `main.ts` to `server/transactions/app.ts` (actualbudget#4221) * Move transaction related handlers to server/transactions folder and use the new convention * Fix lint and typecheck * Release notes * Update handler names * Move get-earliest-transaction * Update release notes * Fix tests * Fix types * Fix lint * Update snapshot * Remove duplicate parse-file.ts * Fix lint * 🐛 Fix `On budget` / `Off budget` underline with translated languages (actualbudget#4417) * Fix `On budget` / `Off budget` underline * md * ajuste para o merge --------- Co-authored-by: Michael Clark <5285928+MikesGlitch@users.noreply.github.com> Co-authored-by: Matt Farrell <10377148+MattFaz@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: youngcw <calebyoung94@gmail.com> Co-authored-by: Matt Fiddaman <github@m.fiddaman.uk> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Martin Michotte <55855805+MMichotte@users.noreply.github.com> Co-authored-by: Joel Jeremy Marquez <joeljeremy.marquez@gmail.com> Co-authored-by: Adam Stück <adam@adast.dk> Co-authored-by: Alberto Cortina Eduarte <albertocortina96@gmail.com> Co-authored-by: Gabriel J. Michael <gabriel.j.michael@gmail.com> Co-authored-by: Julian Dominguez-Schatz <julian.dominguezschatz@gmail.com> Co-authored-by: Michał Gołąb <23549913+michalgolab@users.noreply.github.com> Co-authored-by: Antoine Taillard <an.taillard@gmail.com>
In the course of fixing a bug in #4112 I noticed that the
sendfunction always had a return type ofany. Turns out the function was typed incorrectly and this wasn't caught due to having disabled checks of.d.tsfiles. I'll open a separate PR to test turning on that flag, but using this PR so I can iterate on how CI responds to having proper types forsend.Depends on #4146 and #4147