From d9cc6e7657cee2de9cb5ff2cc76f2e7bfb5bb0e0 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 10 Apr 2024 16:54:24 -0400 Subject: [PATCH] Automatically reset forms after action finishes (#28804) This updates the behavior of form actions to automatically reset the form's uncontrolled inputs after the action finishes. This is a frequent feature request for people using actions and it aligns the behavior of client-side form submissions more closely with MPA form submissions. It has no impact on controlled form inputs. It's the same as if you called `form.reset()` manually, except React handles the timing of when the reset happens, which is tricky/impossible to get exactly right in userspace. The reset shouldn't happen until the UI has updated with the result of the action. So, resetting inside the action is too early. Resetting in `useEffect` is better, but it's later than ideal because any effects that run before it will observe the state of the form before it's been reset. It needs to happen in the mutation phase of the transition. More specifically, after all the DOM mutations caused by the transition have been applied. That way the `defaultValue` of the inputs are updated before the values are reset. The idea is that the `defaultValue` represents the current, canonical value sent by the server. Note: this change has no effect on form submissions that aren't triggered by an action. --- packages/react-art/src/ReactFiberConfigART.js | 1 + .../src/client/ReactFiberConfigDOM.js | 5 ++ .../src/__tests__/ReactDOMForm-test.js | 80 +++++++++++++++++++ .../src/ReactFiberConfigFabric.js | 3 + .../src/ReactFiberConfigNative.js | 3 + .../src/createReactNoop.js | 4 + .../src/ReactFiberCommitWork.js | 53 ++++++++++++ .../react-reconciler/src/ReactFiberFlags.js | 4 +- .../react-reconciler/src/ReactFiberHooks.js | 77 +++++++++++++++++- .../src/forks/ReactFiberConfig.custom.js | 2 + .../src/ReactFiberConfigTestHost.js | 3 + 11 files changed, 230 insertions(+), 5 deletions(-) diff --git a/packages/react-art/src/ReactFiberConfigART.js b/packages/react-art/src/ReactFiberConfigART.js index 0900c50f7e94e..1fee29d43dad0 100644 --- a/packages/react-art/src/ReactFiberConfigART.js +++ b/packages/react-art/src/ReactFiberConfigART.js @@ -490,3 +490,4 @@ export function waitForCommitToBeReady() { } export const NotPendingTransition = null; +export function resetFormInstance() {} diff --git a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js index 91e401b667329..ac76fafd5ad86 100644 --- a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js +++ b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js @@ -3441,3 +3441,8 @@ function insertStylesheetIntoRoot( } export const NotPendingTransition: TransitionStatus = NotPending; + +export type FormInstance = HTMLFormElement; +export function resetFormInstance(form: FormInstance): void { + form.reset(); +} diff --git a/packages/react-dom/src/__tests__/ReactDOMForm-test.js b/packages/react-dom/src/__tests__/ReactDOMForm-test.js index ba721c215fc64..37261020e4627 100644 --- a/packages/react-dom/src/__tests__/ReactDOMForm-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMForm-test.js @@ -39,6 +39,7 @@ describe('ReactDOMForm', () => { let useState; let Suspense; let startTransition; + let use; let textCache; let useFormStatus; let useActionState; @@ -55,6 +56,7 @@ describe('ReactDOMForm', () => { useState = React.useState; Suspense = React.Suspense; startTransition = React.startTransition; + use = React.use; useFormStatus = ReactDOM.useFormStatus; container = document.createElement('div'); document.body.appendChild(container); @@ -1334,4 +1336,82 @@ describe('ReactDOMForm', () => { assertLog(['1']); expect(container.textContent).toBe('1'); }); + + test('uncontrolled form inputs are reset after the action completes', async () => { + const formRef = React.createRef(); + const inputRef = React.createRef(); + const divRef = React.createRef(); + + function App({promiseForUsername}) { + // Make this suspensey to simulate RSC streaming. + const username = use(promiseForUsername); + + return ( +
{ + const rawUsername = formData.get('username'); + const normalizedUsername = rawUsername.trim().toLowerCase(); + + Scheduler.log(`Async action started`); + await getText('Wait'); + + // Update the app with new data. This is analagous to re-rendering + // from the root with a new RSC payload. + startTransition(() => { + root.render( + , + ); + }); + }}> + +
+ +
+ + ); + } + + // Initial render + const root = ReactDOMClient.createRoot(container); + const promiseForInitialUsername = getText('(empty)'); + await resolveText('(empty)'); + await act(() => + root.render(), + ); + assertLog(['Current username: (empty)']); + expect(divRef.current.textContent).toEqual('Current username: (empty)'); + + // Dirty the uncontrolled input + inputRef.current.value = ' AcdLite '; + + // Submit the form. This will trigger an async action. + await submit(formRef.current); + assertLog(['Async action started']); + expect(inputRef.current.value).toBe(' AcdLite '); + + // Finish the async action. This will trigger a re-render from the root with + // new data from the "server", which suspends. + // + // The form should not reset yet because we need to update `defaultValue` + // first. So we wait for the render to complete. + await act(() => resolveText('Wait')); + assertLog([]); + // The DOM input is still dirty. + expect(inputRef.current.value).toBe(' AcdLite '); + // The React tree is suspended. + expect(divRef.current.textContent).toEqual('Current username: (empty)'); + + // Unsuspend and finish rendering. Now the form should be reset. + await act(() => resolveText('acdlite')); + assertLog(['Current username: acdlite']); + // The form was reset to the new value from the server. + expect(inputRef.current.value).toBe('acdlite'); + expect(divRef.current.textContent).toEqual('Current username: acdlite'); + }); }); diff --git a/packages/react-native-renderer/src/ReactFiberConfigFabric.js b/packages/react-native-renderer/src/ReactFiberConfigFabric.js index 0a4f30c901531..f1b6859ab7ea2 100644 --- a/packages/react-native-renderer/src/ReactFiberConfigFabric.js +++ b/packages/react-native-renderer/src/ReactFiberConfigFabric.js @@ -515,6 +515,9 @@ export function waitForCommitToBeReady(): null { export const NotPendingTransition: TransitionStatus = null; +export type FormInstance = Instance; +export function resetFormInstance(form: Instance): void {} + // ------------------- // Microtasks // ------------------- diff --git a/packages/react-native-renderer/src/ReactFiberConfigNative.js b/packages/react-native-renderer/src/ReactFiberConfigNative.js index c24c3a0c95f4f..20a3f150fd111 100644 --- a/packages/react-native-renderer/src/ReactFiberConfigNative.js +++ b/packages/react-native-renderer/src/ReactFiberConfigNative.js @@ -549,3 +549,6 @@ export function waitForCommitToBeReady(): null { } export const NotPendingTransition: TransitionStatus = null; + +export type FormInstance = Instance; +export function resetFormInstance(form: Instance): void {} diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index f64499c6dc801..12b7420dae209 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -90,6 +90,8 @@ type SuspenseyCommitSubscription = { export type TransitionStatus = mixed; +export type FormInstance = Instance; + const NO_CONTEXT = {}; const UPPERCASE_CONTEXT = {}; if (__DEV__) { @@ -632,6 +634,8 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { waitForCommitToBeReady, NotPendingTransition: (null: TransitionStatus), + + resetFormInstance(form: Instance) {}, }; const hostConfig = useMutation diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 213ff26d82e2c..87cbb2c13487c 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -15,6 +15,7 @@ import type { ChildSet, UpdatePayload, HoistableRoot, + FormInstance, } from './ReactFiberConfig'; import type {Fiber, FiberRoot} from './ReactInternalTypes'; import type {Lanes} from './ReactFiberLane'; @@ -97,6 +98,7 @@ import { Visibility, ShouldSuspendCommit, MaySuspendCommit, + FormReset, } from './ReactFiberFlags'; import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber'; import { @@ -163,6 +165,7 @@ import { prepareToCommitHoistables, suspendInstance, suspendResource, + resetFormInstance, } from './ReactFiberConfig'; import { captureCommitPhaseError, @@ -226,6 +229,9 @@ if (__DEV__) { let offscreenSubtreeIsHidden: boolean = false; let offscreenSubtreeWasHidden: boolean = false; +// Used to track if a form needs to be reset at the end of the mutation phase. +let needsFormReset = false; + const PossiblyWeakSet = typeof WeakSet === 'function' ? WeakSet : Set; let nextEffect: Fiber | null = null; @@ -2776,6 +2782,20 @@ function commitMutationEffectsOnFiber( } } } + + if (flags & FormReset) { + needsFormReset = true; + if (__DEV__) { + if (finishedWork.type !== 'form') { + // Paranoid coding. In case we accidentally start using the + // FormReset bit for something else. + console.error( + 'Unexpected host component type. Expected a form. This is a ' + + 'bug in React.', + ); + } + } + } } return; } @@ -2852,6 +2872,21 @@ function commitMutationEffectsOnFiber( } } } + + if (needsFormReset) { + // A form component requested to be reset during this commit. We do this + // after all mutations in the rest of the tree so that `defaultValue` + // will already be updated. This way you can update `defaultValue` using + // data sent by the server as a result of the form submission. + // + // Theoretically we could check finishedWork.subtreeFlags & FormReset, + // but the FormReset bit is overloaded with other flags used by other + // fiber types. So this extra variable lets us skip traversing the tree + // except when a form was actually submitted. + needsFormReset = false; + recursivelyResetForms(finishedWork); + } + return; } case HostPortal: { @@ -3091,6 +3126,24 @@ function commitReconciliationEffects(finishedWork: Fiber) { } } +function recursivelyResetForms(parentFiber: Fiber) { + if (parentFiber.subtreeFlags & FormReset) { + let child = parentFiber.child; + while (child !== null) { + resetFormOnFiber(child); + child = child.sibling; + } + } +} + +function resetFormOnFiber(fiber: Fiber) { + recursivelyResetForms(fiber); + if (fiber.tag === HostComponent && fiber.flags & FormReset) { + const formInstance: FormInstance = fiber.stateNode; + resetFormInstance(formInstance); + } +} + export function commitLayoutEffects( finishedWork: Fiber, root: FiberRoot, diff --git a/packages/react-reconciler/src/ReactFiberFlags.js b/packages/react-reconciler/src/ReactFiberFlags.js index 718add62948e0..49aebe2f9b11c 100644 --- a/packages/react-reconciler/src/ReactFiberFlags.js +++ b/packages/react-reconciler/src/ReactFiberFlags.js @@ -42,6 +42,7 @@ export const StoreConsistency = /* */ 0b0000000000000100000000000000 export const ScheduleRetry = StoreConsistency; export const ShouldSuspendCommit = Visibility; export const DidDefer = ContentReset; +export const FormReset = Snapshot; export const LifecycleEffectMask = Passive | Update | Callback | Ref | Snapshot | StoreConsistency; @@ -95,7 +96,8 @@ export const MutationMask = ContentReset | Ref | Hydrating | - Visibility; + Visibility | + FormReset; export const LayoutMask = Update | Callback | Ref | Visibility; // TODO: Split into PassiveMountMask and PassiveUnmountMask diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index a06303451c49b..b32c9c120166d 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -91,6 +91,7 @@ import { StoreConsistency, MountLayoutDev as MountLayoutDevEffect, MountPassiveDev as MountPassiveDevEffect, + FormReset, } from './ReactFiberFlags'; import { HasEffect as HookHasEffect, @@ -844,15 +845,29 @@ export function TransitionAwareHostComponent(): TransitionStatus { if (!enableAsyncActions) { throw new Error('Not implemented.'); } + const dispatcher: any = ReactSharedInternals.H; const [maybeThenable] = dispatcher.useState(); + let nextState; if (typeof maybeThenable.then === 'function') { const thenable: Thenable = (maybeThenable: any); - return useThenable(thenable); + nextState = useThenable(thenable); } else { const status: TransitionStatus = maybeThenable; - return status; + nextState = status; + } + + // The "reset state" is an object. If it changes, that means something + // requested that we reset the form. + const [nextResetState] = dispatcher.useState(); + const prevResetState = + currentHook !== null ? currentHook.memoizedState : null; + if (prevResetState !== nextResetState) { + // Schedule a form reset + currentlyRenderingFiber.flags |= FormReset; } + + return nextState; } export function checkDidRenderIdHook(): boolean { @@ -2948,7 +2963,30 @@ export function startHostTransition( next: null, }; - // Add the state hook to both fiber alternates. The idea is that the fiber + // We use another state hook to track whether the form needs to be reset. + // The state is an empty object. To trigger a reset, we update the state + // to a new object. Then during rendering, we detect that the state has + // changed and schedule a commit effect. + const initialResetState = {}; + const newResetStateQueue: UpdateQueue = { + pending: null, + lanes: NoLanes, + // We're going to cheat and intentionally not create a bound dispatch + // method, because we can call it directly in startTransition. + dispatch: (null: any), + lastRenderedReducer: basicStateReducer, + lastRenderedState: initialResetState, + }; + const resetStateHook: Hook = { + memoizedState: initialResetState, + baseState: initialResetState, + baseQueue: null, + queue: newResetStateQueue, + next: null, + }; + stateHook.next = resetStateHook; + + // Add the hook list to both fiber alternates. The idea is that the fiber // had this hook all along. formFiber.memoizedState = stateHook; const alternate = formFiber.alternate; @@ -2968,10 +3006,41 @@ export function startHostTransition( NoPendingHostTransition, // TODO: We can avoid this extra wrapper, somehow. Figure out layering // once more of this function is implemented. - () => callback(formData), + () => { + // Automatically reset the form when the action completes. + requestFormReset(formFiber); + return callback(formData); + }, ); } +function requestFormReset(formFiber: Fiber) { + const transition = requestCurrentTransition(); + + if (__DEV__) { + if (transition === null) { + // An optimistic update occurred, but startTransition is not on the stack. + // The form reset will be scheduled at default (sync) priority, which + // is probably not what the user intended. Most likely because the + // requestFormReset call happened after an `await`. + // TODO: Theoretically, requestFormReset is still useful even for + // non-transition updates because it allows you to update defaultValue + // synchronously and then wait to reset until after the update commits. + // I've chosen to warn anyway because it's more likely the `await` mistake + // described above. But arguably we shouldn't. + console.error( + 'requestFormReset was called outside a transition or action. To ' + + 'fix, move to an action, or wrap with startTransition.', + ); + } + } + + const newResetState = {}; + const resetStateHook: Hook = (formFiber.memoizedState.next: any); + const resetStateQueue = resetStateHook.queue; + dispatchSetState(formFiber, resetStateQueue, newResetState); +} + function mountTransition(): [ boolean, (callback: () => void, options?: StartTransitionOptions) => void, diff --git a/packages/react-reconciler/src/forks/ReactFiberConfig.custom.js b/packages/react-reconciler/src/forks/ReactFiberConfig.custom.js index 348a70564c1b7..7ba17939736de 100644 --- a/packages/react-reconciler/src/forks/ReactFiberConfig.custom.js +++ b/packages/react-reconciler/src/forks/ReactFiberConfig.custom.js @@ -39,6 +39,7 @@ export opaque type TimeoutHandle = mixed; // eslint-disable-line no-undef export opaque type NoTimeout = mixed; // eslint-disable-line no-undef export opaque type RendererInspectionConfig = mixed; // eslint-disable-line no-undef export opaque type TransitionStatus = mixed; // eslint-disable-line no-undef +export opaque type FormInstance = mixed; // eslint-disable-line no-undef export type EventResponder = any; export const getPublicInstance = $$$config.getPublicInstance; @@ -78,6 +79,7 @@ export const startSuspendingCommit = $$$config.startSuspendingCommit; export const suspendInstance = $$$config.suspendInstance; export const waitForCommitToBeReady = $$$config.waitForCommitToBeReady; export const NotPendingTransition = $$$config.NotPendingTransition; +export const resetFormInstance = $$$config.resetFormInstance; // ------------------- // Microtasks diff --git a/packages/react-test-renderer/src/ReactFiberConfigTestHost.js b/packages/react-test-renderer/src/ReactFiberConfigTestHost.js index b20d2054f1002..c9474a8950dde 100644 --- a/packages/react-test-renderer/src/ReactFiberConfigTestHost.js +++ b/packages/react-test-renderer/src/ReactFiberConfigTestHost.js @@ -349,3 +349,6 @@ export function waitForCommitToBeReady(): null { } export const NotPendingTransition: TransitionStatus = null; + +export type FormInstance = Instance; +export function resetFormInstance(form: Instance): void {}