Skip to content

Ensure dispatch from useFormState works in StrictMode - #28557

Merged
eps1lon merged 3 commits into
react:mainfrom
eps1lon:strictmode-useformstate
Mar 20, 2024
Merged

eps1lon merged 3 commits into
react:mainfrom
eps1lon:strictmode-useformstate

Conversation

@eps1lon

@eps1lon eps1lon commented Mar 14, 2024 •

Copy link
Copy Markdown
Collaborator

Summary

Closes #28556

We didn't follow the Rules of Hooks in the rerender implementation for mounts.

Test plan

  • Added test based on "useFormState updates state asynchronously and queues multiple actions" but with StrictMode and less dispatches (because it repros on the first attempt)

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 14, 2024
@react-sizebot

ghost commented Mar 14, 2024 •

Copy link
Copy Markdown

Comparing: bbc571a...e32f38a

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 176.83 kB 176.84 kB +0.02% 54.91 kB 54.92 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 173.55 kB 173.55 kB = 54.11 kB 54.11 kB
facebook-www/ReactDOM-prod.classic.js = 594.04 kB 594.07 kB = 104.37 kB 104.38 kB
facebook-www/ReactDOM-prod.modern.js = 577.30 kB 577.33 kB = 101.42 kB 101.43 kB
test_utils/ReactAllWarnings.js Deleted 66.87 kB 0.00 kB Deleted 16.36 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
test_utils/ReactAllWarnings.js Deleted 66.87 kB 0.00 kB Deleted 16.36 kB 0.00 kB

Generated by 🚫 dangerJS against e32f38a

@eps1lon eps1lon changed the title Failing test for null dispatch returned from useFormState in StrictMode Ensure dispatch in useFormState works in StrictMode Mar 14, 2024
@eps1lon
eps1lon force-pushed the strictmode-useformstate branch from f792c17 to 1c4e7c4 Compare March 14, 2024 10:20
@eps1lon
eps1lon force-pushed the strictmode-useformstate branch from 1c4e7c4 to 9b3f1f3 Compare March 14, 2024 10:21
@eps1lon
eps1lon marked this pull request as ready for review March 14, 2024 10:25
@eps1lon
eps1lon requested a review from acdlite March 14, 2024 10:26
@eps1lon eps1lon changed the title Ensure dispatch in useFormState works in StrictMode Ensure dispatch from useFormState works in StrictMode Mar 14, 2024
}

// For mount, pending is always false.
const [isPending] = rerenderState(false);

ghost Mar 19, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Since we don't actually need the result, all we need to do is advance the hook list.

Suggested change
const [isPending] = rerenderState(false);
updateWorkInProgressHook();

ghost Mar 19, 2024

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

ghost Mar 19, 2024

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Feels a bit brittle but you know this part better than I do.

ghost Mar 20, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I consider this pretty low level code so I think it's fine. I care more about not observing the value of isPending since it causes a bunch of redundant read operations.

Comment thread packages/react-reconciler/src/ReactFiberHooks.js Outdated
@acdlite

ghost commented Mar 19, 2024

Copy link
Copy Markdown
Collaborator

This was introduced by #28514. Easy oversight to make, glad we caught it quickly.

Note that it doesn't just affect StrictMode, the same issue happen if you called setState during render to trigger a re-render. Or if you suspended with use after useFormState has already been called. That last one is the most idiomatic so I'd probably write the regression test using that pattern.

Co-Authored-By: Andrew Clark <git@andrewclark.io>
@eps1lon
eps1lon requested a review from acdlite March 19, 2024 18:34
@eps1lon
eps1lon merged commit 8ef14cf into react:main Mar 20, 2024
@eps1lon
eps1lon deleted the strictmode-useformstate branch March 20, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed React Core Team Opened by a member of the React Core Team React 19

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: useFormState formAction becomes null in strict mode

4 participants