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

Mimic class properties with useEventCallback #1566

Merged
merged 3 commits into from May 31, 2019

Conversation

Projects
None yet
3 participants
@jaredpalmer
Copy link
Owner

commented May 30, 2019

In v1, all helper methods we class properties. This meant that they were bound to the instance of the class. However, with hooks, there is no concept of this and each helper method is wrapped in useCallback. However, certain behaviors (e.g. auto-saving) are not possible because of circular useCallback dependencies. For example, submitForm depends on values, so if you change values, then submitForm will be different. This prevents you from debouncing submitForm in response to values because you get a brand new function on each change. Sadness.

The suggested solution in the React docs is to use this hook:

function Form() {
  const [text, updateText] = useState('');
  // Will be memoized even if `text` changes:
  const handleSubmit = useEventCallback(() => {
    alert(text);
  }, [text]);

  return (
    <>
      <input value={text} onChange={e => updateText(e.target.value)} />
      <ExpensiveTree onSubmit={handleSubmit} />
    </>
  );
}

function useEventCallback(fn, dependencies) {
  const ref = useRef(() => {
    throw new Error('Cannot call an event handler while rendering.');
  });

  useEffect(() => {
    ref.current = fn;
  }, [fn, ...dependencies]);

  return useCallback(() => {
    const fn = ref.current;
    return fn();
  }, [ref]);
}

This PR adds this hook internally and utilizes it where necessary (on any callback that relies on state). It also adds a new DebouncedAutoSave example to the examples directory.

@jaredpalmer jaredpalmer requested a review from Andreyco May 30, 2019

@jaredpalmer jaredpalmer merged commit e51f09a into next May 31, 2019

5 checks passed

WIP ready for review
Details
ci/circleci: deploy-docs Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
security/snyk - package.json (jaredpalmer) No new issues
Details
security/snyk - website/package.json (jaredpalmer) No manifest changes detected
@VanTanev

This comment has been minimized.

Copy link

commented Jun 1, 2019

useEventCallback() can be written as

function useEventCallback<T extends (...args: any[]) => any>(
  fn: T
): T {
  const ref: any = React.useRef();

  // we copy a ref to the callback scoped to the current state/props on each render
  React.useLayoutEffect(() => {
    ref.current = fn;
  });

  return React.useCallback((...args) => ref.current.apply(void 0, args), []) as T;
}

as per facebook/react#14099 (comment)

To me this would make it more clear that useEventCallback() is essentially a hack to have a callback which has the entire current state/props, and not only the deps, but the identity of the callback itself doesn't change, making it safe to send downstream and not cause rerenders.

That is to say, even the current version does essentially this, but useLayoutEffect() makes it more explicit than useEffect() + deps + throw on incomplete render.

Edit Another thing that falls off from this change is, now whenever useEventCallback() is used, it looks weird, because it has no deps. And, I think it's good to look weird, because it /is/ weird. It makes it more clear that this isn't a normal hook.

@slorber

This comment has been minimized.

Copy link
Contributor

commented on src/Formik.tsx in 1fd9580 Jun 11, 2019

hey @jaredpalmer , this fixes an important bug on my side where submitForm resolves way too early, would appreciate if you had time to cut a new RC release with that fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.