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

Validations run on unmount creating unnecessary requests #408

Closed
rosskevin opened this issue Jan 2, 2019 · 6 comments · Fixed by #595
Closed

Validations run on unmount creating unnecessary requests #408

rosskevin opened this issue Jan 2, 2019 · 6 comments · Fixed by #595
Labels

Comments

@rosskevin
Copy link
Contributor

rosskevin commented Jan 2, 2019

Are you submitting a bug report or a feature request?

Bug

What is the current behavior?

When leaving a form that is embedded in a dialog, the form is unmounted which in turn calls unsubscribe and ultimately that runs field validations. In the case a user cancels on a complex form with server side validations, it creates a noticeable slowdown of ui as well as unnecessary requests to the server.

dashboard_-_archetype_com

What is the expected behavior?

Unmount will avoid unnecessary work.

Sandbox Link

Reproduction based on the Synchronous field-level validation https://codesandbox.io/s/pw19j1x52m

  1. allow to render initially
  2. clear console
  3. click Unmount form
  4. observe validations firing by added console logs

What's your environment?

Sandbox updated to latest.
ff: 4.11.0
rff: 4.0.2

Other information

possibly related #336

@erikras
Copy link
Member

erikras commented Feb 9, 2019

Hmm... I wonder if unmounting the form needs to pause validation like mounting the form does. I also wonder what order the componentWillUnmount is called, i.e. child-first or parent-first... 🤔

Great reporting, @rosskevin!

@erikras erikras added the bug label Feb 9, 2019
@justingrant
Copy link

justingrant commented May 23, 2019

EDIT: it looks like the infinite loop I ran into (described below) may not be an RFF bug after all, but instead may be a bug in react-router that will be fixed in react-router version 5.0.1. See remix-run/react-router#6673 for the problem and remix-run/react-router#6674 and remix-run/react-router#6690 for the fixes. I'll update this issue if I can repro it on top of the fixed react-router version. But even if this infinite loop isn't RFF's fault doesn't mean that running validation on unmount is a good idea! ;-)

I also have a form within a dialog, and I'm also seeing problems when unmounting, except in my case it's worse: an infinite loop. I'm getting the dreaded Maximum update depth exceeded) error in setState that's called as part of the unmount process. Unmounting triggers validation, which triggers unsubscribing, which in turn triggers setState, which in turn crashes React when nested over 50 levels deep. The setState is inside the cleanup callback that's returned from useEffect() inside the ReactFinalForm function component.

The full error message from React is this: Uncaught Invariant Violation: Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops.

To answer your question above: I don't see why a form should need to run validation when it's unmounting! Even if it didn't cause an infinite loop, some validations can have non-trivial cost, and if the form is unmounting then why would validation be needed?

Also, out of curiosity what is that setState used for? If you're cleaning up subscriptions, why is a state update needed?

Here's the setState call that triggers the problem:

// In the future, changing subscriptions on the fly should be banned. ⚠️
const flattenedSubscription = flattenSubscription(subscription)
React.useEffect(() => {
// We have rendered, so all fields are no registered, so we can unpause validation
form.isValidationPaused() && form.resumeValidation()
const unsubscriptions: Unsubscribe[] = [
form.subscribe(s => {
if (!shallowEqual(s, state)) {
setState(s)
}
}, subscription),
...(decorators
? decorators.map(decorator =>
// this noop ternary is to appease the flow gods
// istanbul ignore next
decorator(form)
)
: [])
]
return () => {
unsubscriptions.forEach(unsubscribe => unsubscribe())
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [decorators, flattenedSubscription])

If it matters, here's some details about how I'm using RFF:

  • My form is in a function component, and I use hooks in the component but don't use any RFF hooks.
  • The problem reproes with the current latest version of RFF (5.1.0). I also tried the new flattenSubscription PR Make flattenSubscription hooks-safe #479 from @Andarist but that didn't fix the problem.
  • The problem happens whether or not I submit the form (running the submit handler) or simply cancel out of the form.
  • Unlike the OP of this issue, my validation functions are synchronous, fast, and local. My onSubmit handler is an async function, but I don't think this is related because the problem also happens with the Cancel button which is a sync handler: <Button onClick={() => setDone(true)}>
  • I'm not using any fancy features of RFF like FormSpy. I'm only using fields, validation, pristine, and submitting.
  • I tried running my code inside or outside of a <StrictMode> tree and it didn't affect the problem.
  • The problem does not interfere with the onSubmit handler, which successfully runs to completion before the infinite loop happens.
  • As far as I can tell, the only significant difference between this form and others in my app that run fine is that this is running inside a dialog. I adapted this form's code from another hooks-using function component which does not live in a dialog (but is otherwise nearly identical to the dialog version, including unmounting via a react-router redirect) and the problem doesn't happen there.
  • I'll try to build a simple repro of the issue if I can. Gotta rip out a lot of code so may take a while.
  • The dialog component that I'm using is the Modal component from reactstrap, but the way I handle dialogs is by pushing them into a global state object that I monitor for changes (via a custom hook). Because the form content inside the dialog also depends on the same global state object, it's likely that unmounting the dialog triggers a re-render of the form inside the dialog. If I had to guess, it's this behavior that's exposing the infinite loop behavior in RFF. UPDATE: the problem still happens even when I remove my global state hook. Interestingly, none of the other dialog-hosting pages in my app have any problems. It's only when RFF forms are put inside a dialog where the issue shows up.
  • Unmounting is triggered by a (hooks-based) setState call in my code. I have a const [done, setDone] = useState(false); at the top of my function component, and in onSubmit (and the click handler for the Cancel button) I call setDone(true). My function component uses conditional rendering based on the value of the done state variable. If it's false, then the form is rendered. If true, then I use react-router to redirect to a new page, which in turn triggers the unmount. Here's an excerpt of the rendering code with extraneous stuff removed.
  return done ? (
    <Redirect to={'/times'} />
  ) : (
    <>
      <Form
        onSubmit={async (values) => onSubmit(values)}
        initialValues={initialValues}
        render={({ handleSubmit, pristine, invalid }) => {
          return (
            <form onSubmit={handleSubmit}
              <FieldContainer>
                <FieldLabel>Date:</FieldLabel>
                <Field name="date" validate={isDateObjectOrStringValid}>
                  {({ input, meta }) => (
                    <>
                      <DatePickerAdapter {...input} />
                      {meta.error && meta.touched && <span style={{ color: 'red' }}>{meta.error}</span>}
                    </>
                  )}
                </Field>
              </FieldContainer>
              {/* omitted three form fields to keep this excerpt short */}
              <FieldContainer>
                <Button type="submit" disabled={pristine || invalid}>Save</Button>
                <Button onClick={() => setDone(true)}>Cancel</Button>
              </FieldContainer>
            </form>
          );

Here's most of the call stack except the bottom few frames which are probably not relevant:

image

Here's another view of the call stack, including the "component stack":
image

@shrugs
Copy link

shrugs commented Jul 31, 2019

I also ran into this bug, since we do cryptographic options to validate a form, when a form is unmounted (even post-submission), the unregistrations cause validation to run once for every field that was unmounted (including useField hooks that are only subscribing to state).

My solution is:

import { useEffect } from 'react';
import { useForm } from 'react-final-form';

/**
 * pauses validation when it becomes unmounted
 *
 * NOTE: this is useful because for some reason, final-form will
 * re-run validation when a field is unregistered (even post submission !?),
 * which react-final-form does on unmount. So when we redirect away from
 * a page with a form on it, all of the validation is re-triggered.
 *
 * Normally, for boring sync-validation-only situations, this is ok, but for
 * long-running validators like ours (deriving seeds, checking chain, etc)
 * having them re-triggered when a form leaves the page is hilariously bad.
 *
 * So here we disable validation when unmounting, saving ourselves
 * from the footgun.
 */
export default function ValidationPauser() {
  const form = useForm();

  useEffect(() => () => form.pauseValidation(), [form]);

  return null;
}

which should be placed as the first component within a Form's children.

@asazernik
Copy link
Contributor

@erikras - according to this very very old comment, as of late 2015 componentWillUnmount() was called parent-first, which should make things easy; I'll try pauseValidation() in componentWillUnmount() of the top-level form component and see if that fixes the issue for me.

@asazernik
Copy link
Contributor

Worked like magic!

@erikras
Copy link
Member

erikras commented Nov 18, 2019

Published fix in v6.3.1.

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

Successfully merging a pull request may close this issue.

5 participants