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.

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

Automatically reset forms after action finishes #28804

Merged
merged 1 commit into from
Apr 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions packages/react-art/src/ReactFiberConfigART.js
Original file line number Diff line number Diff line change
Expand Up @@ -490,3 +490,4 @@ export function waitForCommitToBeReady() {
}

export const NotPendingTransition = null;
export function resetFormInstance() {}
5 changes: 5 additions & 0 deletions packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js
Original file line number Diff line number Diff line change
Expand Up @@ -3441,3 +3441,8 @@ function insertStylesheetIntoRoot(
}

export const NotPendingTransition: TransitionStatus = NotPending;

export type FormInstance = HTMLFormElement;
export function resetFormInstance(form: FormInstance): void {
form.reset();
}
80 changes: 80 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMForm-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ describe('ReactDOMForm', () => {
let useState;
let Suspense;
let startTransition;
let use;
let textCache;
let useFormStatus;
let useActionState;
Expand All @@ -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);
Expand Down Expand Up @@ -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 (
<form
ref={formRef}
action={async formData => {
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(
<App promiseForUsername={getText(normalizedUsername)} />,
);
});
}}>
<input
ref={inputRef}
text="text"
name="username"
defaultValue={username}
/>
<div ref={divRef}>
<Text text={'Current username: ' + username} />
</div>
</form>
);
}

// Initial render
const root = ReactDOMClient.createRoot(container);
const promiseForInitialUsername = getText('(empty)');
await resolveText('(empty)');
await act(() =>
root.render(<App promiseForUsername={promiseForInitialUsername} />),
);
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');
});
});
3 changes: 3 additions & 0 deletions packages/react-native-renderer/src/ReactFiberConfigFabric.js
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,9 @@ export function waitForCommitToBeReady(): null {

export const NotPendingTransition: TransitionStatus = null;

export type FormInstance = Instance;
export function resetFormInstance(form: Instance): void {}

// -------------------
// Microtasks
// -------------------
Expand Down
3 changes: 3 additions & 0 deletions packages/react-native-renderer/src/ReactFiberConfigNative.js
Original file line number Diff line number Diff line change
Expand Up @@ -549,3 +549,6 @@ export function waitForCommitToBeReady(): null {
}

export const NotPendingTransition: TransitionStatus = null;

export type FormInstance = Instance;
export function resetFormInstance(form: Instance): void {}
4 changes: 4 additions & 0 deletions packages/react-noop-renderer/src/createReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ type SuspenseyCommitSubscription = {

export type TransitionStatus = mixed;

export type FormInstance = Instance;

const NO_CONTEXT = {};
const UPPERCASE_CONTEXT = {};
if (__DEV__) {
Expand Down Expand Up @@ -632,6 +634,8 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
waitForCommitToBeReady,

NotPendingTransition: (null: TransitionStatus),

resetFormInstance(form: Instance) {},
};

const hostConfig = useMutation
Expand Down
53 changes: 53 additions & 0 deletions packages/react-reconciler/src/ReactFiberCommitWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import type {
ChildSet,
UpdatePayload,
HoistableRoot,
FormInstance,
} from './ReactFiberConfig';
import type {Fiber, FiberRoot} from './ReactInternalTypes';
import type {Lanes} from './ReactFiberLane';
Expand Down Expand Up @@ -97,6 +98,7 @@ import {
Visibility,
ShouldSuspendCommit,
MaySuspendCommit,
FormReset,
} from './ReactFiberFlags';
import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber';
import {
Expand Down Expand Up @@ -163,6 +165,7 @@ import {
prepareToCommitHoistables,
suspendInstance,
suspendResource,
resetFormInstance,
} from './ReactFiberConfig';
import {
captureCommitPhaseError,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should start a campaign for javascript64

// 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: {
Expand Down Expand Up @@ -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,
Expand Down
4 changes: 3 additions & 1 deletion packages/react-reconciler/src/ReactFiberFlags.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
77 changes: 73 additions & 4 deletions packages/react-reconciler/src/ReactFiberHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ import {
StoreConsistency,
MountLayoutDev as MountLayoutDevEffect,
MountPassiveDev as MountPassiveDevEffect,
FormReset,
} from './ReactFiberFlags';
import {
HasEffect as HookHasEffect,
Expand Down Expand Up @@ -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<TransitionStatus> = (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 {
Expand Down Expand Up @@ -2948,7 +2963,30 @@ export function startHostTransition<F>(
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<Object, Object> = {
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;
Expand All @@ -2968,10 +3006,41 @@ export function startHostTransition<F>(
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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this actually have to be on the Fiber?

Can't it just be scheduled on the Root with a list of forms to reset when the root commits?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That way you don't need all the special commit phase stuff.

Copy link
Collaborator Author

@acdlite acdlite Apr 10, 2024

Choose a reason for hiding this comment

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

Hmm I'll need to think more about that, the scenario I had in mind was if the form is inside a Suspense boundary, the root could commit before the input's defaultValue had been updated. Usually this shouldn't happen because during a Transition we won't hide already visible forms, but during a popstate transition we might.

The <input form="myform"> case is interesting there, too.

}

function mountTransition(): [
boolean,
(callback: () => void, options?: StartTransitionOptions) => void,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
Loading