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

Inference of Hooks’ inputs #14406

Closed
yuchi opened this issue Dec 8, 2018 · 12 comments
Closed

Inference of Hooks’ inputs #14406

yuchi opened this issue Dec 8, 2018 · 12 comments
Labels
Component: Hooks Resolution: Stale Automatically closed due to inactivity

Comments

@yuchi
Copy link

yuchi commented Dec 8, 2018

I open this issue (as suggested by @gaearon on Twitter) to discuss how Hook’s inputs argument should be inferred.

Background

The current Hooks API reference cites (emphasis mine):

The array of inputs is not passed as arguments to the function. Conceptually, though, that’s what they represent: every value referenced inside the function should also appear in the inputs array. In the future, a sufficiently advanced compiler could create this array automatically.

Captivated by this sentence (and the idea of bringing “the future” closer) I built hooks.macro, a Babel Macro which tries to do exactly this. That is, to infer the second argument to useMemo, useCallback and use*Effect hooks.

The tool trasform this:

import { useAutoMemo } from 'hooks.macro';
function MyComponent({ propValue }) {
  return useAutoMemo(() => propValue.split('').join('-'));
  //     ---^^^^----                                    ^ no inputs
}

To this:

function MyComponent({ propValue }) {
  return React.useMemo(() => propValue.split('').join('-'), [propValue]);
  //     ^---^ ^-----^                                       ^-------^
}

In the process of designing the constraints of that tool, I realized there are more than a few open questions, whose answers can be found only with guidance from the core React team and a broader discussion with the community.

Rationale

Why should we bother with this topic? The reasons that motivated me in implementing hooks.macro were the following.

  1. Failing to populate the inputs array correctly has sad consequences:

    1. too wide, and it will invalidate the use of memoization itself, and/or bring potential perf issues;

    2. too narrow, and bugs can be tricky to identify (IMHO unit tests are not viable to avoid this: you need to test changes, not just different possible values;)

    3. bugs caused by having non local values are potentially non deterministic.

  2. Keeping the inputs array updated is important, but it’s a matter of discipline:

    1. when I change the “body” of the hook I need to check it;

    2. when I change the semantics of a used valued I need to verify if I need to include it (e.g. when it was a constant and now a prop);

    3. not introducing bugs during merge conflicts is matter of even higher discipline (since usually inputs will fit in a single line, you probably need to edit the line to solve the conflict).

  3. By removing the burden of adding the inputs array, we reduce the mental overhead of using the memoizing hooks (namely useMemo and useCallback). This let us:

    1. make following the best practices easier,

    2. encourage the use of memoization in unexpected places (such as directly inside the final returning JSX of a component).

  4. Nothing is stopping me from shooting my own feet with obvious errors (such as passing the result of a non-memoized array literal ad an input). If a stable enough approach is found, this could be even solved automatically by memoizing all necessary inputs too.

  5. It’s an error-prone and boring practice that can be automated away.

Current implementation

(Straight from hooks.macro’s README)

Features

  1. Extracts all references used, and adds them to the inputs array.

  2. Favors strict correctness over performance, but uses safe optimizations:

    1. skips constants and useless inputs;

    2. traverses all functions called or referenced, and appends their dependencies too, removing the need for unnecessary useCallback hooks.

Constraints

  1. Only variables created in the scope of the component body are automatically trapped as inputs.

  2. Only variables, and not properties’ access, are trapped. This means that if you use obj.prop only [obj] will become part of the memoization invalidation keys. This is a problem for refs, and will be addressed specifically in a future release.

    You can work around this limitation by creating a variable which holds the current value, such as const { current } = ref.

  3. Currently there’s no way to add additional keys for more fine grained cache invalidation. Could be an important escape hatch when you do nasty things, but in that case I’d prefer to use useMemo/useCallback directly.

  4. Only locally defined functions declarations and explicit function expressions (let x = () => {}) are traversed for indirect dependencies — all other function calls (such as xxx()) are treated as normal input dependencies and appended too. This is unnecessary (but not harmful) for setters coming from useState, and not an issue at all if the function is the result of useCallback or useAutoCallback.

Questions

As I said before I have few open questions for the core React team, but I’m sure the whole community will be impactful in the discussion.

  1. What were/are the requirements of the «sufficiently advanced compiler» you envision? Is the current approach of hooks.macro aligned with those?

  2. Using a broader approach, Flow/TypeScript hints could give an incredible amount of precision not available on the pure Babel pipeline. I feel pre-pack can help here too (we could help pre-pack do its thing actually) but I’m not sure how.

  3. Are users potentially scared by the amount of black magic involved? I tried to follow the Hooks lead by having very clear, very understandable rules. Is this enough?

  4. The auto-closure feature of useMemo (you don’t need an arrow function) has received the most negative feedback in the small discussions I had online. Yet I personally find the most interesting hook of this library, since adds so little overhead I can throw it any time I need… potentially abusing it eventually (so, points against it?)

  5. I personally prefer to have a different API that makes it clear that those are not standard hooks without "inputs". A Babel plugin that transform all hooks without an explicit inputs array seems silly and scary — you get used to not passing them and you don't have the API signaling you that you need to.

  6. Since this is a specialized form of lambda-lifting, are you willing to add some advanced hooks (with one of your lovely silly prefixes such as DONT_USE_) that pass the inputs as arguments to creator/effect function? This open some interesting perf optimization opportunities by hoisting functions higher up.

@Jessidhia
Copy link
Contributor

Jessidhia commented Dec 10, 2018

Note that ref updates never trigger a rerender. Because of that, having ref.current as an input is not that useful -- the hook will only run on the next time the component updates.

I find myself typically using refs specifically when you need something that has referential identity while being able to manipulate its .current; or when writing custom hooks where I am emulating a useState-like API where the second result is always the same:

const realCallback = useCallback(() => {/* ... */}, [/* real inputs */])
const callbackRef = useRef(realCallback)
// useLayoutEffect for updating this could be better
// but it'd break synchronous invocations
// right now it breaks concurrent updates instead 🤷‍♀️
callbackRef.current = realCallback
// note how memoCallback is only ever supposed to be generated once
const memoCallback = useCallback<typeof realCallback>(() => callbackRef.current(), [callbackRef])
// memoCallback is returned as the 2nd item of the pair

Although all that this is is a horrible workaround for the problem in #14099


TypeScript hints can't really help with keeping the inputs array consistent; at best they can just tell when you tried to use something in an input while it's in TDZ (because you are following rules of hooks, right?).


What do you mean by auto-callback feature? You mean it's possible to pass a non-function to useMemo and it'll treat it as the callback's result? 😥

Personally my problem with useMemo is with its sibling, useCallback. Calling useCallback without an inputs array is the same thing as not using useCallback at all, just with more overhead.

@yuchi
Copy link
Author

yuchi commented Dec 10, 2018

Note that ref updates never trigger a rerender.

Yes and no, see how it is used in useOnScreen in this example. There are two latent bugs in that code: 1) when the same ref is used on a different element the hook will not update the listener and 2) if for some reasons the ref is not filled when the effect is run (it should be) the observer is simply never attached.

That specific case should support a pass-through inputs array that somehow encodes that the ref refers to something else (a responsibility completely in the hands of the users of the hook). Having ref.current as input would not help at all (since refs are obviously not filled at render and at the second re-render it would uselessly destroy the observer).

Should the second argument of effects support an inputs array creator? Looks silly IMHO.
Or should we avoid useRef in favor of useState (at the cost of a useless re-render) when the ref is potentially dynamic? If so, custom hooks hooks sometimes should be called with:

const [element, setElement] = useState(null);
const onScreen = useOnScreen(useAutoMemo({ current: element })); // <-- auto-callback in use here
return <div ref={setElement}></div>

These complexities root on a still incomplete grasp of hooks from me (but I hope/fear I’m not alone) and made me wait on adding object properties access in inputs. Lets wait the core team to shed some light.


TypeScript hints can't really help with keeping the inputs array consistent;

We can do more aggressive optimizations. If I could know that a value is a ReadOnlyArray then I could spread the values in the inputs array too, and I could remove the need for a useMemo call:

const { values: ReadOnlyArray<string> } = props;
return useAutoMemo(values.join('-'));
// ↓ ↓ ↓
return useMemo(() => values.join('-'), [values.length, ...values]);
Note that values.length there is for paranoid-mode. Click for more details.

Since I could have multiple arrays I could have ambiguous situations:

// First run:
const a = [1, 2, 3];
const b = [4, 5, 6];
// Second run
const a = [1, 2];
const b = [3, 4, 5, 6];

// Ambiguous:
useMemo(() => a.reduce(sum) - b.reduce(sum), [...a, ...b]);
// on first _and_ second run: [1, 2, 3, 4, 5, 6]

// Unambiguous:
useMemo(() => a.reduce(sum) - b.reduce(sum), [a.length, ...a, b.length ...b]);
// on first run: [3, 1, 2, 3, 4, 4, 5, 6]
// on second run: [2, 1, 2, 4, 3, 4, 5, 6]

If I know that a value will always have different values I can print a warning and opt-out of an inputs array entirely. Not sure how to identify this with Flow or TypeScript, pre-pack can help here.


What do you mean by auto-callback feature? You mean it's possible to pass a non-function to useMemo and it'll treat it as the callback's result? 😥

Yes, exactly. You can do the following:

const memoized = useAutoMemo(value);
// ↓ ↓ ↓
const memoized = useMemo(() => value, [value]);

This horribly opinionated and I got negative feedback around it — but I’m gonna keep it around for a little bit for few simple reasons:

  1. useMemo creator function is called contextually/synchronously (when is called at all) so the semantic is not so terribly off;
  2. it reduces boilerplate and makes adding memoization just a matter of
    1. select
    2. wrap with parens
    3. move the cursor to the left
    4. add useAutoMemo;
  3. Reduces reading overhead and makes placing it inside JSX more pleasurable.

@yuchi
Copy link
Author

yuchi commented Dec 10, 2018

Personally my problem with useMemo is with its sibling, useCallback. Calling useCallback without an inputs array is the same thing as not using useCallback at all, just with more overhead.

I already filed an issue on hooks.macro to track the idea of having a generic auto entry point that could help with a hypothetical useStableCallback:

const [something, setSomething] = useState();
const handleChange = useStableCallback(...auto(event => {
  setSomethingElse(something + parseInt(event.currentTarget.value, 10));
}));
// ↓ ↓ ↓
const handleChange = useStableCallback(event => {
  setSomethingElse(something + parseInt(event.currentTarget.value, 10));
},  [something]);

(Thanks for the feedback!)

@Jessidhia
Copy link
Contributor

Jessidhia commented Dec 11, 2018

@yuchi your useAutoMemo(value) is just the same thing as the current useCallback(value) but with double evaluation 🤔

useCallback(arg) is the same as useMemo(() => arg, [arg]) (but only evaluates arg once), which is why I argue that useCallback without the inputs array is a waste of CPU time. If your arg is already referentially stable it'll never update so you never needed to memoize it; if it's not then it'll always update and defeat the memoization.

@yuchi
Copy link
Author

yuchi commented Dec 11, 2018

@yuchi your useAutoMemo(value) is just the same thing as the current useCallback(value) but with double evaluation 🤔

Can you elaborate a little bit? I’m not sure I understood what you mean. useAutoMemo(value) is exactly like useMemo(() => value, [value]) but with the arrow “unwrapped”. That specific example is silly and useless I know.

@yuchi
Copy link
Author

yuchi commented Dec 18, 2018

It just occurred to me that the term “auto-callback” (to refer to the fact that with useAutoMemo you don’t need an arrow) is… misleading at best. Sorry if this caused confusion. It comes from a Swift feature which is called “auto-closure”, but I recalled it wrong.

@stale
Copy link

stale bot commented Jan 10, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 10, 2020
@denis-sokolov
Copy link
Contributor

This is a thorough suggestion and I would appreciate hearing an opinion and guidance from the core team. Please, stalebot.

@stale
Copy link

stale bot commented Apr 9, 2020

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Apr 9, 2020
@yuchi
Copy link
Author

yuchi commented Apr 14, 2020

Bump

@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Apr 14, 2020
@stale
Copy link

stale bot commented Jul 13, 2020

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jul 13, 2020
@stale
Copy link

stale bot commented Jul 20, 2020

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

@stale stale bot closed this as completed Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Hooks Resolution: Stale Automatically closed due to inactivity
Projects
None yet
Development

No branches or pull requests

4 participants