Skip to content
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.

Sign up for GitHub

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

Fix top-level types of send function #4145

Merged
merged 7 commits into from
Feb 19, 2025
Merged

Fix top-level types of send function #4145

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d5ea716
Add release notes
jfdoming Jan 14, 2025
c17b83e
Fix types of `send` function
jfdoming Jan 14, 2025
84fd2b8
Fix `send` types in a number of places
jfdoming Jan 14, 2025
30c2e28
PR feedback
jfdoming Jan 23, 2025
3102013
Merge branch 'master' into jfdoming/fix-send-types
jfdoming Jan 23, 2025
eda6f09
Merge branch 'master' into jfdoming/fix-send-types
jfdoming Feb 5, 2025
94b0a0f
Merge branch 'master' into jfdoming/fix-send-types
jfdoming Feb 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 14 additions & 17 deletions packages/desktop-client/src/components/modals/EditAccess.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import { Trans, useTranslation } from 'react-i18next';
import { addNotification, popModal, signOut } from 'loot-core/client/actions';
import { send } from 'loot-core/platform/client/fetch';
import { getUserAccessErrors } from 'loot-core/shared/errors';
import { type Handlers } from 'loot-core/types/handlers';
import { type UserAccessEntity } from 'loot-core/types/models/userAccess';

import { useDispatch } from '../../redux';
Expand Down Expand Up @@ -34,22 +33,20 @@ export function EditUserAccess({
const [availableUsers, setAvailableUsers] = useState<[string, string][]>([]);

useEffect(() => {
send('access-get-available-users', defaultUserAccess.fileId).then(
(data: Awaited<ReturnType<Handlers['access-get-available-users']>>) => {
if ('error' in data) {
setSetError(data.error);
} else {
setAvailableUsers(
data.map(user => [
user.userId,
user.displayName
? `${user.displayName} (${user.userName})`
: user.userName,
]),
);
}
},
);
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,
]),
);
}
});
Comment on lines +36 to +49
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

}, [defaultUserAccess.fileId]);

async function onSave(close: () => void) {
Expand Down
10 changes: 4 additions & 6 deletions packages/loot-core/src/platform/client/fetch/index.browser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,9 @@ export const init: T.Init = async function (worker) {
};

export const send: T.Send = function (
name,
args,
{ catchErrors = false } = {},
) {
...params: Parameters<T.Send>
): ReturnType<T.Send> {
const [name, args, { catchErrors = false } = {}] = params;
return new Promise((resolve, reject) => {
const id = uuidv4();

Expand All @@ -167,8 +166,7 @@ export const send: T.Send = function (
} else {
globalWorker.postMessage(message);
}
// eslint-disable-next-line @typescript-eslint/no-explicit-any
}) as any;
});
};

export const sendCatch: T.SendCatch = function (name, args) {
Expand Down
24 changes: 15 additions & 9 deletions packages/loot-core/src/platform/client/fetch/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,25 +7,31 @@ export type Init = typeof init;

export function send<K extends keyof Handlers>(
name: K,
args?: Parameters<Handlers[K]>[0],
options?: { catchErrors: true },
): ReturnType<
| { data: Handlers[K] }
| { error: { type: 'APIError' | 'InternalError'; message: string } }
args: Parameters<Handlers[K]>[0],
options: { catchErrors: true },
): Promise<
| { data: Awaited<ReturnType<Handlers[K]>>; error: undefined }
| {
data: undefined;
error: { type: 'APIError' | 'InternalError'; message: string };
}
>;
export function send<K extends keyof Handlers>(
name: K,
args?: Parameters<Handlers[K]>[0],
options?: { catchErrors?: boolean },
): ReturnType<Handlers[K]>;
): Promise<Awaited<ReturnType<Handlers[K]>>>;
export type Send = typeof send;

export function sendCatch<K extends keyof Handlers>(
name: K,
args?: Parameters<Handlers[K]>[0],
): ReturnType<
| { data: Handlers[K] }
| { error: { type: 'APIError' | 'InternalError'; message: string } }
): Promise<
| { data: Awaited<ReturnType<Handlers[K]>>; error: undefined }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| { data: Awaited<ReturnType<Handlers[K]>>; error: undefined }
| { data: Awaited<ReturnType<Handlers[K]>> }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. In that case we should probably just merge those two properties and make them both optional. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 };
Comment on lines +30 to +33
Copy link
Contributor

@joel-jeremy joel-jeremy Jan 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| { 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; }

}
Comment on lines +31 to +34
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| {
data: undefined;
error: { type: 'APIError' | 'InternalError'; message: string };
}
| {
error: { type: 'APIError' | 'InternalError'; message: string };
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as above

>;
export type SendCatch = typeof sendCatch;

Expand Down
7 changes: 3 additions & 4 deletions packages/loot-core/src/platform/client/fetch/index.web.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,9 @@ export const init: T.Init = async function () {
};

export const send: T.Send = function (
name,
args,
{ catchErrors = false } = {},
) {
...params: Parameters<T.Send>
): ReturnType<T.Send> {
const [name, args, { catchErrors = false } = {}] = params;
return new Promise((resolve, reject) => {
const id = uuidv4();
replyHandlers.set(id, { resolve, reject });
Expand Down
6 changes: 6 additions & 0 deletions upcoming-release-notes/4145.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
category: Maintenance
authors: [jfdoming]
---

Fix types of `send` function
Loading