From be93fbcb82f01d38ae9f8cf2ba330404058a0502 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Tue, 19 Sep 2023 17:19:30 +0200 Subject: [PATCH] [v14] Add error to Attempt in useAsync (#32117) * Add error to Attempt in useAsync * Ensure attempt.error refers to the same object as expectedError * Refactor error handling to use instanceof with attempt.error * makeErrorAttempt: Require an Error object In real code, the function can throw any object. But since makeErrorAttempt is a helper function, we can make it require an Error object. --- web/packages/shared/hooks/useAsync.test.ts | 23 +++++++ web/packages/shared/hooks/useAsync.ts | 66 ++++++++++++++++--- .../ClusterLogin/ClusterLogin.story.tsx | 9 +-- .../DocumentConnectMyComputerSetup.tsx | 2 +- .../DocumentConnectMyComputerStatus.tsx | 8 +-- .../connectMyComputerContext.test.tsx | 2 +- .../DocumentGateway/DocumentGateway.story.tsx | 12 ++-- .../ui/DocumentTerminal/Terminal/Terminal.tsx | 4 +- 8 files changed, 96 insertions(+), 30 deletions(-) diff --git a/web/packages/shared/hooks/useAsync.test.ts b/web/packages/shared/hooks/useAsync.test.ts index befda0368f009..69d87ee8505a8 100644 --- a/web/packages/shared/hooks/useAsync.test.ts +++ b/web/packages/shared/hooks/useAsync.test.ts @@ -171,3 +171,26 @@ test('run does not update state after being re-run when the callback returns a r await secondRunPromise; }); }); + +test('error and statusText are set when the callback returns a rejected promise', async () => { + const expectedError = new Error('whoops'); + + const { result, waitFor } = renderHook(() => + useAsync(() => Promise.reject(expectedError)) + ); + + let runPromise: Promise; + let [, run] = result.current; + + act(() => { + runPromise = run(); + }); + await waitFor(() => result.current[0].status === 'error'); + + const attempt = result.current[0]; + expect(attempt['error']).toBe(expectedError); + expect(attempt['statusText']).toEqual(expectedError.message); + + // The promise returned from run always succeeds, but any errors are captured as the second arg. + await expect(runPromise).resolves.toEqual([null, expectedError]); +}); diff --git a/web/packages/shared/hooks/useAsync.ts b/web/packages/shared/hooks/useAsync.ts index 43ce02fdbdc90..6a53cc324537f 100644 --- a/web/packages/shared/hooks/useAsync.ts +++ b/web/packages/shared/hooks/useAsync.ts @@ -93,8 +93,9 @@ export function useAsync( const run = useCallback( (...args: Args) => { setState(prevState => ({ - ...prevState, status: 'processing', + data: prevState['data'], + statusText: prevState['statusText'], })); const promise = cb(...args); @@ -125,9 +126,9 @@ export function useAsync( return [null, new CanceledError()] as [AttemptData, Error]; } - setState(prevState => ({ - ...prevState, + setState(() => ({ status: 'error', + error: err, statusText: err?.message, data: null, })); @@ -163,13 +164,43 @@ export class CanceledError extends Error { } } -export type AttemptStatus = 'processing' | 'success' | 'error' | ''; +export type AttemptStatus = Attempt['status']; -export type Attempt = { - data?: T; - status: AttemptStatus; - statusText: string; -}; +export type Attempt = + | { + status: ''; + data: null; + /** + * @deprecated statusText is present for compatibility purposes only. To use statusText, check + * if status equals 'error' first. + */ + statusText: string; + } + | { + status: 'processing'; + /** data is either null or contains data from the previous success attempt if the attempt was retried. */ + data: null | T; + /** + * @deprecated statusText is present for compatibility purposes only. To use statusText, check + * if status equals 'error' first. + */ + statusText: string; + } + | { + status: 'success'; + data: T; + /** + * @deprecated statusText is present for compatibility purposes only. To use statusText, check + * if status equals 'error' first. + */ + statusText: string; + } + | { + status: 'error'; + data: null; + statusText: string; + error: any; + }; export function hasFinished(attempt: Attempt): boolean { return attempt.status === 'success' || attempt.status === 'error'; @@ -199,11 +230,26 @@ export function makeProcessingAttempt(): Attempt { }; } -export function makeErrorAttempt(statusText: string): Attempt { +export function makeErrorAttempt(error: Error): Attempt { + return { + data: null, + status: 'error', + error: error, + statusText: error.message, + }; +} + +/** + * @deprecated Use makeErrorAttempt instead. + */ +export function makeErrorAttemptWithStatusText( + statusText: string +): Attempt { return { data: null, status: 'error', statusText, + error: new Error(statusText), }; } diff --git a/web/packages/teleterm/src/ui/ClusterConnect/ClusterLogin/ClusterLogin.story.tsx b/web/packages/teleterm/src/ui/ClusterConnect/ClusterLogin/ClusterLogin.story.tsx index ff574e213c1a7..22dc64c0e91b6 100644 --- a/web/packages/teleterm/src/ui/ClusterConnect/ClusterLogin/ClusterLogin.story.tsx +++ b/web/packages/teleterm/src/ui/ClusterConnect/ClusterLogin/ClusterLogin.story.tsx @@ -17,7 +17,7 @@ limitations under the License. import React from 'react'; import { Box } from 'design'; -import { Attempt } from 'shared/hooks/useAsync'; +import { Attempt, makeErrorAttempt } from 'shared/hooks/useAsync'; import * as types from 'teleterm/ui/services/clusters/types'; import { @@ -70,12 +70,9 @@ function makeProps(): ClusterLoginPresentationProps { }; } -export const Error = () => { +export const Err = () => { const props = makeProps(); - props.initAttempt = { - status: 'error', - statusText: 'some error message', - }; + props.initAttempt = makeErrorAttempt(new Error('some error message')); return ( diff --git a/web/packages/teleterm/src/ui/ConnectMyComputer/DocumentConnectMyComputerSetup/DocumentConnectMyComputerSetup.tsx b/web/packages/teleterm/src/ui/ConnectMyComputer/DocumentConnectMyComputerSetup/DocumentConnectMyComputerSetup.tsx index 1dbd13dbf0f5f..c17d0469a9240 100644 --- a/web/packages/teleterm/src/ui/ConnectMyComputer/DocumentConnectMyComputerSetup/DocumentConnectMyComputerSetup.tsx +++ b/web/packages/teleterm/src/ui/ConnectMyComputer/DocumentConnectMyComputerSetup/DocumentConnectMyComputerSetup.tsx @@ -192,7 +192,7 @@ function AgentSetup({ rootClusterUri }: { rootClusterUri: RootClusterUri }) { return; } - if (joinClusterAttempt.statusText !== AgentProcessError.name) { + if (!(joinClusterAttempt.error instanceof AgentProcessError)) { return ; } diff --git a/web/packages/teleterm/src/ui/ConnectMyComputer/DocumentConnectMyComputerStatus/DocumentConnectMyComputerStatus.tsx b/web/packages/teleterm/src/ui/ConnectMyComputer/DocumentConnectMyComputerStatus/DocumentConnectMyComputerStatus.tsx index 69e63c2af1c5a..61130bbac16b9 100644 --- a/web/packages/teleterm/src/ui/ConnectMyComputer/DocumentConnectMyComputerStatus/DocumentConnectMyComputerStatus.tsx +++ b/web/packages/teleterm/src/ui/ConnectMyComputer/DocumentConnectMyComputerStatus/DocumentConnectMyComputerStatus.tsx @@ -258,7 +258,7 @@ function prettifyCurrentAction(currentAction: CurrentAction): { return noop; // noop, not used, at this point it should be start processing. } default: { - return assertUnreachable(currentAction.attempt.status); + return assertUnreachable(currentAction.attempt); } } } @@ -272,7 +272,7 @@ function prettifyCurrentAction(currentAction: CurrentAction): { }; } case 'error': { - if (currentAction.attempt.statusText !== AgentProcessError.name) { + if (!(currentAction.attempt.error instanceof AgentProcessError)) { return { Icon: StyledWarning, title: 'Failed to start agent', @@ -306,7 +306,7 @@ function prettifyCurrentAction(currentAction: CurrentAction): { return noop; // noop, not used, at this point it should be observe-process running. } default: { - return assertUnreachable(currentAction.attempt.status); + return assertUnreachable(currentAction.attempt); } } break; @@ -377,7 +377,7 @@ function prettifyCurrentAction(currentAction: CurrentAction): { return noop; // noop, not used, at this point it should be observe-process exited. } default: { - return assertUnreachable(currentAction.attempt.status); + return assertUnreachable(currentAction.attempt); } } } diff --git a/web/packages/teleterm/src/ui/ConnectMyComputer/connectMyComputerContext.test.tsx b/web/packages/teleterm/src/ui/ConnectMyComputer/connectMyComputerContext.test.tsx index 72c7259f5fb5a..18a232989e0d6 100644 --- a/web/packages/teleterm/src/ui/ConnectMyComputer/connectMyComputerContext.test.tsx +++ b/web/packages/teleterm/src/ui/ConnectMyComputer/connectMyComputerContext.test.tsx @@ -117,7 +117,7 @@ test('startAgent re-throws errors that are thrown while spawning the process', a expect(error).toBeInstanceOf(AgentProcessError); expect(result.current.currentAction).toStrictEqual({ kind: 'start', - attempt: makeErrorAttempt(AgentProcessError.name), + attempt: makeErrorAttempt(new AgentProcessError()), agentProcessState: { status: 'error', message: 'ENOENT', diff --git a/web/packages/teleterm/src/ui/DocumentGateway/DocumentGateway.story.tsx b/web/packages/teleterm/src/ui/DocumentGateway/DocumentGateway.story.tsx index 3cb633d68e66e..cf76d00225b73 100644 --- a/web/packages/teleterm/src/ui/DocumentGateway/DocumentGateway.story.tsx +++ b/web/packages/teleterm/src/ui/DocumentGateway/DocumentGateway.story.tsx @@ -19,7 +19,7 @@ import React from 'react'; import { makeEmptyAttempt, makeProcessingAttempt, - makeErrorAttempt, + makeErrorAttemptWithStatusText, makeSuccessAttempt, } from 'shared/hooks/useAsync'; @@ -91,7 +91,7 @@ export function OnlineWithFailedDbNameAttempt() { return ( ( + changeDbNameAttempt={makeErrorAttemptWithStatusText( 'Something went wrong with setting database name.' )} /> @@ -102,7 +102,7 @@ export function OnlineWithFailedPortAttempt() { return ( ( + changePortAttempt={makeErrorAttemptWithStatusText( 'Something went wrong with setting port.' )} /> @@ -113,10 +113,10 @@ export function OnlineWithFailedDbNameAndPortAttempts() { return ( ( + changeDbNameAttempt={makeErrorAttemptWithStatusText( 'Something went wrong with setting database name.' )} - changePortAttempt={makeErrorAttempt( + changePortAttempt={makeErrorAttemptWithStatusText( 'Something went wrong with setting port.' )} /> @@ -142,7 +142,7 @@ export function OfflineWithFailedConnectAttempt() { gateway={undefined} defaultPort="62414" connected={false} - connectAttempt={makeErrorAttempt( + connectAttempt={makeErrorAttemptWithStatusText( 'listen tcp 127.0.0.1:62414: bind: address already in use' )} /> diff --git a/web/packages/teleterm/src/ui/DocumentTerminal/Terminal/Terminal.tsx b/web/packages/teleterm/src/ui/DocumentTerminal/Terminal/Terminal.tsx index cfcecd04f1674..c83b1fcc2e21d 100644 --- a/web/packages/teleterm/src/ui/DocumentTerminal/Terminal/Terminal.tsx +++ b/web/packages/teleterm/src/ui/DocumentTerminal/Terminal/Terminal.tsx @@ -21,7 +21,7 @@ import { debounce } from 'shared/utils/highbar'; import { Attempt, makeEmptyAttempt, - makeErrorAttempt, + makeErrorAttemptWithStatusText, makeSuccessAttempt, } from 'shared/hooks/useAsync'; @@ -59,7 +59,7 @@ export function Terminal(props: TerminalProps) { useEffect(() => { const removeOnStartErrorListener = props.ptyProcess.onStartError( message => { - setStartPtyProcessAttempt(makeErrorAttempt(message)); + setStartPtyProcessAttempt(makeErrorAttemptWithStatusText(message)); } );