final-form / react-final-form

🏁 High performance subscription-based form state management for React
https://final-form.org/react
MIT License
7.39k stars 481 forks source link

Bug: useFormState implementation may swallow state updates #784

Open SleepWalker opened 4 years ago

SleepWalker commented 4 years ago

Are you submitting a bug report or a feature request?

bug report

What is the current behavior?

Current implementation of useFormState hook is not reliable. State initialization and useEffect hook are called asynchronously:

https://github.com/final-form/react-final-form/blob/abd6d6266d2f6953a1995cb3dedd26f198e653fc/src/useFormState.js#L19-L46

This means, that between this two calls, the form state may be changed. And yes, it happens sometimes :(

What is the expected behavior?

When you using useFormState hook it should replicate all calls of form.subscribe()'s callback!

I think, that firstRender.current is a bad way for skipping needless state updates. Because this may cause skipping of really important updates from subscription. Instead I suggest the following implementation:

function useFormState<FormValues = any>({
  onChange,
  subscription = all,
}: UseFormStateParams<FormValues> = {}): FormState<FormValues> {
  const form: FormApi<FormValues> = useForm<FormValues>('useFormState');
  const unsubscribe = React.useRef(() => {});

  // synchronously register and unregister to query field state for our subscription on first render
  const [state, setState] = React.useState<FormState<FormValues>>(
    (): FormState<FormValues> => {
      let initialState: FormState<FormValues>;
      let initialRender = true;

      unsubscribe.current = form.subscribe((state) => {
        if (initialRender) {
          initialState = state;
          initialRender = false;
        } else {
          setState(state);
        }

        if (onChange) {
          onChange(state);
        }
      }, subscription);

      return initialState;
    },
  );

  React.useEffect(
    () => unsubscribe.current,
    // eslint-disable-next-line react-hooks/exhaustive-deps
    [],
  );

  return state;
}

Sandbox Link

skrivanos commented 4 years ago

Ran into this problem as well. Manifested itself in a Submit component using the hook not getting the correct invalid state on initial render:

image

@SleepWalker's suggestion didn't solve the problem, but a massively simplified version of the hook did (basically just a useEffect-wrapper around form.subscribe and calling form.getState() to get the initial state). Since I assume there's a good reason for the complexity of the original hook I won't share the code here since it could have unforeseen consequences.