From ebfcc0902b33d98f728bf58afa491c248899c017 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 31 May 2024 16:30:20 -0400 Subject: [PATCH] useActionState: On error, cancel remaining actions If an action in the useActionState queue errors, we shouldn't run any subsequent actions. The contract of useActionState is that the actions run in sequence, and that one action can assume that all previous actions have completed successfully. For example, in a shopping cart UI, you might dispatch an "Add to cart" action followed by a "Checkout" action. If the "Add to cart" action errors, the "Checkout" action should not run. An implication of this change is that once useActionState falls into an error state, the only way to recover is to reset the component tree, i.e. by unmounting and remounting. The way to customize the error handling behavior is to wrap the action body in a try/catch. --- .../src/__tests__/ReactDOMForm-test.js | 85 ++++++++++++------- .../react-reconciler/src/ReactFiberHooks.js | 41 ++++----- 2 files changed, 77 insertions(+), 49 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMForm-test.js b/packages/react-dom/src/__tests__/ReactDOMForm-test.js index e38dc244f652d..fbae2805bad88 100644 --- a/packages/react-dom/src/__tests__/ReactDOMForm-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMForm-test.js @@ -1237,14 +1237,12 @@ describe('ReactDOMForm', () => { // @gate enableAsyncActions test('useActionState: error handling (sync action)', async () => { - let resetErrorBoundary; class ErrorBoundary extends React.Component { state = {error: null}; static getDerivedStateFromError(error) { return {error}; } render() { - resetErrorBoundary = () => this.setState({error: null}); if (this.state.error !== null) { return ; } @@ -1284,31 +1282,16 @@ describe('ReactDOMForm', () => { 'Caught an error: Oops!', ]); expect(container.textContent).toBe('Caught an error: Oops!'); - - // Reset the error boundary - await act(() => resetErrorBoundary()); - assertLog(['A']); - - // Trigger an error again, but this time, perform another action that - // overrides the first one and fixes the error - await act(() => { - startTransition(() => action('Oops!')); - startTransition(() => action('B')); - }); - assertLog(['Pending A', 'B']); - expect(container.textContent).toBe('B'); }); // @gate enableAsyncActions test('useActionState: error handling (async action)', async () => { - let resetErrorBoundary; class ErrorBoundary extends React.Component { state = {error: null}; static getDerivedStateFromError(error) { return {error}; } render() { - resetErrorBoundary = () => this.setState({error: null}); if (this.state.error !== null) { return ; } @@ -1346,21 +1329,65 @@ describe('ReactDOMForm', () => { await act(() => resolveText('Oops!')); assertLog(['Caught an error: Oops!', 'Caught an error: Oops!']); expect(container.textContent).toBe('Caught an error: Oops!'); + }); + + test('useActionState: when an action errors, subsequent actions are canceled', async () => { + class ErrorBoundary extends React.Component { + state = {error: null}; + static getDerivedStateFromError(error) { + return {error}; + } + render() { + if (this.state.error !== null) { + return ; + } + return this.props.children; + } + } + + let action; + function App() { + const [state, dispatch, isPending] = useActionState(async (s, a) => { + Scheduler.log('Start action: ' + a); + const text = await getText(a); + if (text.endsWith('!')) { + throw new Error(text); + } + return text; + }, 'A'); + action = dispatch; + const pending = isPending ? 'Pending ' : ''; + return ; + } - // Reset the error boundary - await act(() => resetErrorBoundary()); + const root = ReactDOMClient.createRoot(container); + await act(() => + root.render( + + + , + ), + ); assertLog(['A']); - // Trigger an error again, but this time, perform another action that - // overrides the first one and fixes the error - await act(() => { - startTransition(() => action('Oops!')); - startTransition(() => action('B')); - }); - assertLog(['Pending A']); - await act(() => resolveText('B')); - assertLog(['B']); - expect(container.textContent).toBe('B'); + await act(() => startTransition(() => action('Oops!'))); + assertLog(['Start action: Oops!', 'Pending A']); + + // Queue up another action after the one will error. + await act(() => startTransition(() => action('Should never run'))); + assertLog([]); + + // The first dispatch will update the pending state. + await act(() => resolveText('Oops!')); + assertLog(['Caught an error: Oops!', 'Caught an error: Oops!']); + expect(container.textContent).toBe('Caught an error: Oops!'); + + // Attempt to dispatch another action. This should not run either. + await act(() => + startTransition(() => action('This also should never run')), + ); + assertLog([]); + expect(container.textContent).toBe('Caught an error: Oops!'); }); // @gate enableAsyncActions diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 93c676f020535..de85c8246cb6c 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -1965,7 +1965,9 @@ type ActionStateQueue = { dispatch: Dispatch

, // This is the most recent action function that was rendered. It's updated // during the commit phase. - action: (Awaited, P) => S, + // If it's null, it means the action queue errored and subsequent actions + // should not run. + action: ((Awaited, P) => S) | null, // This is a circular linked list of pending action payloads. It incudes the // action that is currently running. pending: ActionStateQueueNode | null, @@ -2004,9 +2006,15 @@ function dispatchActionState( throw new Error('Cannot update form state while rendering.'); } + const currentAction = actionQueue.action; + if (currentAction === null) { + // An earlier action errored. Subsequent actions should not run. + return; + } + const actionNode: ActionStateQueueNode = { payload, - action: actionQueue.action, + action: currentAction, next: (null: any), // circular isTransition: true, @@ -2189,28 +2197,21 @@ function onActionError( actionNode: ActionStateQueueNode, error: mixed, ) { - actionNode.status = 'rejected'; - actionNode.reason = error; - notifyActionListeners(actionNode); - - // Pop the action from the queue and run the next pending action, if there - // are any. - // TODO: We should instead abort all the remaining actions in the queue. + // Mark all the following actions as rejected. const last = actionQueue.pending; + actionQueue.pending = null; if (last !== null) { const first = last.next; - if (first === last) { - // This was the last action in the queue. - actionQueue.pending = null; - } else { - // Remove the first node from the circular queue. - const next = first.next; - last.next = next; - - // Run the next action. - runActionStateAction(actionQueue, next); - } + do { + actionNode.status = 'rejected'; + actionNode.reason = error; + notifyActionListeners(actionNode); + actionNode = actionNode.next; + } while (actionNode !== first); } + + // Prevent subsequent actions from being dispatched. + actionQueue.action = null; } function notifyActionListeners(actionNode: ActionStateQueueNode) {