jaredpalmer / formik

Build forms in React, without the tears 😭
https://formik.org
Apache License 2.0
33.98k stars 2.79k forks source link

updates during validation on unmounted component #3418

Open artola opened 2 years ago

artola commented 2 years ago

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 commented 2 years ago

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

artola commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 :smile:

artola commented 2 years ago

@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: https://github.com/testing-library/react-testing-library/issues/999

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

wwahammy commented 2 years ago

Thanks for clarifying @artola!

gaearon commented 2 years ago

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

artola commented 2 years ago

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 />,
      ),
    );