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

Inconsistencies in the way how inputs are handled in some hooks #78

Closed
malte-wessel opened this issue Dec 18, 2018 · 5 comments
Closed

Comments

@malte-wessel
Copy link
Contributor

malte-wessel commented Dec 18, 2018

Explain the problem

We have some inconsistencies in the way how inputs (second argument of useEffect, useMemo, useCallback) are handled

Expected Behaviour

All hooks should have the same way of handling inputs. In React useEffect, useMemo, useCallback all have the same API:

// useEffect
// called on mount & update
useEffect(fn);
// called on mount
useEffect(fn, []);
// called on mount & when input values change
useEffect(fn, [a, b]);

// Internal implementation
const dirty =
    !inputs || (inputs.length && !shallowEqualsArray(dataPrev, inputs));

Actual Behaviour

// useEffect
// called on mount & update
useEffect(fn);
// called on mount
useEffect(fn, []);
// called on mount & when input values change
useEffect(fn, [a, b]);

// internal implementation
const dirty =
    !inputs || (inputs.length && !shallowEqualsArray(dataPrev, inputs));

// useCallback
// fn stored on mount (not stored on update! different from useEffect)
useCallback(fn);
// fn stored on mount
useCallback(fn, []);
// fn stored on mount & when input values change
useCallback(fn, [a, b]);

// internal implementation
const dirty =
    inputs && inputs.length && !shallowEqualsArray(inputsPrev, inputs);

// useMemo
// called on mount (not on update! different from useEffect)
useMemo(fn);
// called on mount
useMemo(fn, []);
// called on mount & when input values change, unmount
useMemo(fn, [a, b]);

// internal implementation
const dirty =
    inputs && inputs.length && !shallowEqualsArray(inputsPrev, inputs);

Maybe I'm missing something, but I think there was no good reason why these hooks have a different API. We should make them consistent. I'm happy to create a PR. What do you think?

@malte-wessel
Copy link
Contributor Author

What react does is, if you don't pass inputs it takes the first argument of the hook as input:

// useCallback
const inputsNext = inputs !== undefined && inputs !== null ? inputs : [callback];
const dirty = shallowEqualsArray(inputsNext, inputsPrev);

@jamesb3ll
Copy link

jamesb3ll commented Dec 18, 2018

Hey Malte 😃I've created a codesandbox to see React's behavior (open the console):
https://codesandbox.io/s/52ylkpl26p

From what I can see:

  • useEffect(fn => cleanup(), inputs) will run fn on mount. Also, the cleanup function and fn will run on every update if inputs is an array of values that have changed (or is undefined).
  • useMemo(fn, inputs) will re-run fn every time the inputs changes (or is undefined).
  • useCallback(fn, inputs) is equal to useMemo(() => fn, inputs).

If any of the above are passed a constant array (eg. [] or ['constValue']) as inputs, then fn will only run once.

If on a re-render any of the above have inputs === undefined and fn is changed, the new fn will be run, so useEffect(fn) is actually useEffect(fn, [fn]).

I've not seen Melody's behaviour, but I presume it is different right now?

@pago
Copy link
Contributor

pago commented Dec 19, 2018

I think it makes sense to align the behaviour. That'd make it more predictable.

@malte-wessel
Copy link
Contributor Author

Hey @jamesb3ll 😍
yes, that's the behaviour that makes sense and that we also should implement. I will create a PR soon

@pago
Copy link
Contributor

pago commented Jan 13, 2019

Has been fixed in #80

@pago pago closed this as completed Jan 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants