Skip to content

Fix form state reset when component state is updated #34075

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
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
69 changes: 69 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMForm-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2281,4 +2281,73 @@ describe('ReactDOMForm', () => {
await submit(formRef.current);
assertLog(['stringified action']);
});

it('form actions should retain status when nested state changes', async () => {
const formRef = React.createRef();
const textRef = React.createRef();
const btnRef = React.createRef();

let rerenderUnrelatedStatus;
function UnrelatedStatus() {
const {pending} = useFormStatus();
const [counter, setCounter] = useState(0);
rerenderUnrelatedStatus = () => setCounter(n => n + 1);
Scheduler.log(`[unrelated form] pending: ${pending}, state: ${counter}`);
}

function Status() {
const {pending} = useFormStatus();
const [counter, setCounter] = useState(0);
return (
<div>
<p ref={textRef}>
{pending ? `Pending` : null} with state: {counter}
</p>
<button
ref={btnRef}
onClick={() => setCounter(n => n + 1)}
type="button">
Increment
</button>
</div>
);
}

function App() {
async function action() {
return new Promise(resolve => {
// never resolves
});
}

return (
<>
<form action={action} ref={formRef}>
<input type="submit" />
<Status />
</form>
<form>
<UnrelatedStatus />
</form>
</>
);
}

const root = ReactDOMClient.createRoot(container);
await act(() => root.render(<App />));

assertLog(['[unrelated form] pending: false, state: 0']);

await submit(formRef.current);

expect(textRef.current.textContent).toBe('Pending with state: 0');

await act(() => btnRef.current.click());

expect(textRef.current.textContent).toBe('Pending with state: 1');

await act(() => rerenderUnrelatedStatus());

assertLog(['[unrelated form] pending: false, state: 1']);
});
});
25 changes: 3 additions & 22 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,6 @@ import {
isPrimaryRenderer,
getResource,
createHoistableInstance,
HostTransitionContext,
} from './ReactFiberConfig';
import type {ActivityInstance, SuspenseInstance} from './ReactFiberConfig';
import {shouldError, shouldSuspend} from './ReactFiberReconciler';
Expand Down Expand Up @@ -1870,8 +1869,6 @@ function updateHostComponent(
tryToClaimNextHydratableInstance(workInProgress);
}

pushHostContext(workInProgress);

const type = workInProgress.type;
const nextProps = workInProgress.pendingProps;
const prevProps = current !== null ? current.memoizedProps : null;
Expand Down Expand Up @@ -1899,31 +1896,15 @@ function updateHostComponent(
//
// Once a fiber is upgraded to be stateful, it remains stateful for the
// rest of its lifetime.
const newState = renderTransitionAwareHostComponentWithHooks(
renderTransitionAwareHostComponentWithHooks(
current,
workInProgress,
renderLanes,
);

// If the transition state changed, propagate the change to all the
// descendents. We use Context as an implementation detail for this.
//
// This is intentionally set here instead of pushHostContext because
// pushHostContext gets called before we process the state hook, to avoid
// a state mismatch in the event that something suspends.
Comment on lines -1911 to -1913
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure I fully understand this comment. If something suspends we pop back up. And the Component we're rendering (here: TransitionAwareHostComponent) should not be able to read the current Context just yet which is what pushing the context before the render would do, right? We can't nest Host Context anyway though. So this seems safe to move the push after the render.

//
// NOTE: This assumes that there cannot be nested transition providers,
// because the only renderer that implements this feature is React DOM,
// and forms cannot be nested. If we did support nested providers, then
// we would need to push a context value even for host fibers that
// haven't been upgraded yet.
if (isPrimaryRenderer) {
HostTransitionContext._currentValue = newState;
} else {
HostTransitionContext._currentValue2 = newState;
}
}

pushHostContext(workInProgress);

markRef(current, workInProgress);
reconcileChildren(current, workInProgress, nextChildren, renderLanes);
return workInProgress.child;
Expand Down
4 changes: 2 additions & 2 deletions packages/react-reconciler/src/ReactFiberHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -853,8 +853,8 @@ export function renderTransitionAwareHostComponentWithHooks(
current: Fiber | null,
workInProgress: Fiber,
lanes: Lanes,
): TransitionStatus {
return renderWithHooks(
): void {
renderWithHooks(
current,
workInProgress,
TransitionAwareHostComponent,
Expand Down
21 changes: 20 additions & 1 deletion packages/react-reconciler/src/ReactFiberHostContext.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@

import type {Fiber} from './ReactInternalTypes';
import type {StackCursor} from './ReactFiberStack';
import type {Container, HostContext} from './ReactFiberConfig';
import type {
Container,
HostContext,
TransitionStatus,
} from './ReactFiberConfig';
import type {Hook} from './ReactFiberHooks';

import {
Expand Down Expand Up @@ -92,6 +96,21 @@ function getHostContext(): HostContext {
function pushHostContext(fiber: Fiber): void {
const stateHook: Hook | null = fiber.memoizedState;
if (stateHook !== null) {
// Propagate the state to all the descendents.
// We use Context as an implementation detail for this.
//
// NOTE: This assumes that there cannot be nested transition providers,
// because the only renderer that implements this feature is React DOM,
// and forms cannot be nested. If we did support nested providers, then
// we would need to push a context value even for host fibers that
// haven't been upgraded yet.
const transitionStatus: TransitionStatus = stateHook.memoizedState;
if (isPrimaryRenderer) {
HostTransitionContext._currentValue = transitionStatus;
} else {
HostTransitionContext._currentValue2 = transitionStatus;
}

// Only provide context if this fiber has been upgraded by a host
// transition. We use the same optimization for regular host context below.
push(hostTransitionProviderCursor, fiber, fiber);
Expand Down
Loading