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

updates during validation on unmounted component #3418

Open
artola opened this issue Nov 27, 2021 · 8 comments
Open

updates during validation on unmounted component #3418

artola opened this issue Nov 27, 2021 · 8 comments

Comments

@artola
Copy link
Contributor

artola commented Nov 27, 2021

Bug report

Current Behavior

When used the prop validateOnMount={true} the component runs the validation as required (using validate and/or validationSchema produces the same behaviour), but if the component is unmounted asap it performs some state updates (dispatch) even when an unmounted component should not.

Expected behavior

Do not change state once unmounted.

Reproducible example

https://codesandbox.io/s/formik-codesandbox-template-bug-ku21b?file=/index.test.js

See console error:

Warning: An update to Formik inside a test was not wrapped in act(...).

When testing, code that causes React state updates should be wrapped into act(...):

act(() => {
  /* fire events that update state */
});
/* assert on the output */

This ensures that you're testing the behavior the user would see in the browser. Learn more at https://reactjs.org/link/wrap-tests-with-act
    at Formik (https://ku21b.csb.app/node_modules/formik/dist/formik.esm.js:1062:19)
    at MyForm

Suggested solution(s)

At least in both places where it was used useEffect (with validateOnMount as dependency) should be replaced by useLayoutEffect.

https://github.com/formium/formik/blob/b9cc2536a1edb9f2d69c4cd20ecf4fa0f8059ade/packages/formik/src/Formik.tsx#L335
https://github.com/formium/formik/blob/b9cc2536a1edb9f2d69c4cd20ecf4fa0f8059ade/packages/formik/src/Formik.tsx#L412

With this change, it works as expected for the example provided. It is not clear to me if some other places also should change to useLayoutEffect, specially because of React v17 different handling of them:

https://github.com/formium/formik/blob/b9cc2536a1edb9f2d69c4cd20ecf4fa0f8059ade/packages/formik/src/Formik.tsx#L165

  React.useEffect(() => {
    isMounted.current = true;

    return () => {
      isMounted.current = false;
    };
  }, []);

Additional context

React v17

Your environment

Software Version(s)
Formik 2.2.9
React 17.0.2
@testing-library/react 12.1.2
@johnrom
Copy link
Collaborator

johnrom commented Nov 29, 2021

This might be fixed in #3231 just because the underlying state manager useSubscription checks for unmounts, I think.

@artola
Copy link
Contributor Author

artola commented Nov 29, 2021

@johnrom It might be that v3 version fix it, but it is not released and we should have a quick-fix for v2 time.
@jaredpalmer Should we replace in v2 the above mentioned useEffect (may some other also) by useIsomorphicLayoutEffect because of React v17?

@mikoz93
Copy link

mikoz93 commented Dec 1, 2021

Is there any eta on v3? Seems like it would fix a bunch of people's problems and enahnce existing use-cases so it's a shame that it's kind of being left in limbo for so long now. 🤔

@wwahammy
Copy link
Contributor

wwahammy commented Dec 3, 2021

Just as an FYI for anyone who has seen this bug, I've discovered that if you put the render inside an act, the warnings will go away.

So you can replace

render(<ComponentContainingFormik/>)

with:

await act(async () => render(<ComponentContainingFormik/>))

And the warning go away. At least in my case 😄

@artola
Copy link
Contributor Author

artola commented Dec 3, 2021

@wwahammy With your "solution" the warning is not printed, but the code is still running. I found that it is related to Promise.all used in the validation.
If we do not unmount or cleanup inside the same test case, because even if RTL adds the cleanup to afterEach, it seems not enough.

See: testing-library/react-testing-library#999

I hope the @gaearon could help us to understand it.

@wwahammy
Copy link
Contributor

wwahammy commented Dec 3, 2021

Thanks for clarifying @artola!

@gaearon
Copy link

gaearon commented Dec 4, 2021

I’m on a vacation. If there’s some problem or question about React please file an issue on React issue tracker.

@artola
Copy link
Contributor Author

artola commented May 5, 2022

I am using another workaround, might help someone:

declare module 'react-dom/test-utils' {
  export function act<T>(callback: () => Promise<T>): Promise<T>;
}
  it('should render', async () => {
    const {getByTestId} = await act(async () =>
      render(
        <Foo />,
      ),
    );

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants