Fix incorrect type definitions in useCallback

[mununki]

This fixes the incorrect type definitions of React.useCallback7, 8.
The duplicated polymorphic type 'f between the callback function and item in deps causes the type error.

[cristianoc]

@cknitt is there a better way than using all these versions?

[cknitt]

@mununki Ooops! Thanks for the fix!

Maybe something like

external useCallback6: ('callback, ('a, 'b, 'c, 'd, 'e, 'f)) => 'callback = "useCallback"

would be nicer than

external useCallback6: ('f, ('a, 'b, 'c, 'd, 'e, 'f0)) => 'f = "useCallback"

though?

[mununki]

@cknitt is there a better way than using all these versions?

This reminds me of something: do we really need to use useCallback0-7 to make the number of dependent arrays type safe?

[cknitt]

@cknitt is there a better way than using all these versions?

I was toying with the idea of introducing an abstract type for dependency arrays so that you would write

React.useCallback(() => myCallback(a, b, c, d), React.deps4(a, b, c, d))
React.useEffect(() => myEffect(a), React.dep(a))
React.useMemo(() => myFunction(a, b), React.deps2(a, b))

Not sure if that would be an improvement though...

[mununki]

@cknitt is there a better way than using all these versions?

I was toying with the idea of introducing an abstract type for dependency arrays so that you would write

React.useCallback(() => myCallback(a, b, c, d), React.deps4(a, b, c, d))
React.useEffect(() => myEffect(a), React.dep(a))
React.useMemo(() => myFunction(a, b), React.deps2(a, b))

Not sure if that would be an improvement though...

Oh, I meant using just array:

React.useCallback(_ => f, [a,b,c])

Ah, I'd like to draw my idea. We have to use the tuple, not array for different types of items.

[cristianoc]

Well if you have deps, then it can as well take a generic 'a so it does not constrain what you pass.
What exactly is and is not allowed as dependency?

[cknitt]

Oh, I meant using just array:

React.useCallback(_ => f, [a,b,c])

This will only work if a, b, c have the same type.

[cristianoc]

The array forces you to inject everything into a single type, which complicates things.

[cknitt]

The bindings are trying to enforce that the dependency array is in fact an array.

If we just type the dependency array as 'deps instead then you can pass what you want:

React.useCallback(() => myCallback(a, b, c, d), (a, b, c, d))
React.useEffect(() => myEffect(a), [a])
React.useMemo(() => myFunction(a, b), (a, b))

[cknitt]

But that would be less safe and allow people to pass

React.useEffect(() => myEffect(a), a)

instead of

React.useEffect(() => myEffect(a), [a])

for example.

[cristianoc]

The bindings are trying to enforce that the dependency array is in fact an array.

If we just type the dependency array as 'deps instead then you can pass what you want:

React.useCallback(() => myCallback(a, b, c, d), (a, b, c, d))
React.useEffect(() => myEffect(a), [a])
React.useMemo(() => myFunction(a, b), (a, b))

Is there any serious risk of passing something that is not allowed i.e. is not picked up, or is it pretty clear this is low risk in practice?

[mununki]

Is there any change to improve using unboxed variant instead 'deps?

[cometkim]

React team actually working on some compiler for it (codename: Forget)
https://react.dev/blog/2022/06/15/react-labs-what-we-have-been-working-on-june-2022#react-compiler

So.. we may end up needing PPX or something to build equivalents.

[cristianoc]

E.g. one possible mistake would be to pass a list, but I'm not sure we want to worry about that.
In future, we can always add a more precise type to the language.

[mununki]

Maybe heterogeneous array for deps.

[cknitt]

Ok, what happens if I pass for example a number instead of an array is that I get this warning:

Warning: useMemo received a final argument that is not an array (instead, received number). When specified, the final argument must be an array.

[cknitt]

Currently, in addition to the useCallback1, 2, ... we have this binding

@module("react")
external useCallback: 'f => 'f = "useCallback"

which does not really make sense, as without a dependency array nothing is memoized.

So we could just change that one to

@module("react")
external useCallback: ('f, 'deps) => 'f = "useCallback"

so that we have a less safe, but more convenient option in addition to the useCallback1, 2, ...

[mununki]

Currently, in addition to the useCallback1, 2, ... we have this binding

@module("react")
external useCallback: 'f => 'f = "useCallback"

which does not really make sense, as without a dependency array nothing is memoized.

So we could just change that one to

@module("react")
external useCallback: ('f, 'deps) => 'f = "useCallback"

so that we have a less safe, but more convenient option in addition to the useCallback1, 2, ...

How about removing the useCallback? It seems to me that useCallbacks0-7 are the ones that guarantee there are deps.

[mununki]

I've tried to define a heterogeneous list something like:

type rec dep<'b, 'c> =
  | Nil: dep<'b, 'b>
  | Cons('a, dep<'b, 'c>): dep<'a, 'b>

let x = Cons(1, Cons("a", Cons(1.5, Nil)))

But it doesn't compile with @unboxed.

[mununki]

Currently, in addition to the useCallback1, 2, ... we have this binding

@module("react")
external useCallback: 'f => 'f = "useCallback"

which does not really make sense, as without a dependency array nothing is memoized.

So we could just change that one to

@module("react")
external useCallback: ('f, 'deps) => 'f = "useCallback"

so that we have a less safe, but more convenient option in addition to the useCallback1, 2, ...

f1422b4

[mununki]

Same goes to useEffect and useMemo?

[cknitt]

How about removing the useCallback? It seems to me that useCallbacks0-7 are the ones that guarantee there are deps.

Same goes to useEffect and useMemo?

Same thing for all three, we have two options:

1. Remove the binding without a dependency array
2. Add a dependency array to that binding, typed as 'deps for convenience.

I understood @cristianoc that he would like something like option 2 as we are not too worried about errors here:

E.g. one possible mistake would be to pass a list, but I'm not sure we want to worry about that.
In future, we can always add a more precise type to the language.

Usage would be like this:

React.useCallback(() => myCallback(a, b, c, d), (a, b, c, d))
React.useEffect(() => myEffect(a), [a])
React.useMemo(() => myFunction(a, b), (a, b))

[cknitt]

Ah, sorry, I see you implemented that already, but removed the existing 0-7.

I would suggest to keep them for compatibility with existing projects and maybe deprecate them later on.

[cristianoc]

[cknitt]

One possibility would be to have a descriptive, untagged variant with 2 cases. Might or might not be better.

Then I'd rather have two separate bindings.

In any case, we need to keep in mind that useEffect without a dependency array is very probably not what you want or should be using anyway. I do not think we are using this in any of our apps and cannot think of a use case for it.

I think we should just have

external useEffect: (@uncurry (unit => option<unit => unit>), 'deps) => unit = "useEffect"

as suggested and not even document case 1 (how to pass undefined) at all.

[mununki]

One possibility would be to have a descriptive, untagged variant with 2 cases. Might or might not be better.

Then I'd rather have two separate bindings.

In any case, we need to keep in mind that useEffect without a dependency array is very probably not what you want or should be using anyway. I do not think we are using this in any of our apps and cannot think of a use case for it.

I think we should just have

external useEffect: (@uncurry (unit => option<unit => unit>), 'deps) => unit = "useEffect"

as suggested and not even document case 1 (how to pass undefined) at all.

I know this is mostly unused, but I think it's still worth mentioning in the documentation, especially compared to cases like useCallback or useMemo where the second argument is absolutely necessary.

[cristianoc]

Perhaps keeping the binding as simple as possible and put all the required info in the doc comment, could be the way.

[mununki]

It would be a good use case of untagged variant, but we need one more case for the heterogeneous array or list.

[cknitt]

We can also define something like

type runEffectOnEveryRender

@val
external runEffectOnEveryRender: runEffectOnEveryRender = "undefined"

giving us the following options to document:

• React.runEffectOnEveryRender
• []
• [a]
• (a, b, ...)

[mununki]

Since we've already adopted simplicity and flexibility, what about the documentation explains to use unit for not having a dependencies?

[cknitt]

Since we've already adopted simplicity and flexibility, what about the documentation explains to use unit for not having a dependencies?

That's certainly an option, I am just afraid that people might start to confuse the () and [] usage.

[cristianoc]

I think the tradeoffs are clear. It looks like the case of omitting the second arg cannot be left out without making things worse (People reading the React docs will follow what is recommended).

So the only question is how to represent that case out of the ways considered.
I don't have strong opinions either way.

[cristianoc]

"If you want to skip the effect on initial render and run only on updates, you could use a ref to track if it's the first render"

I'm not sure I know what to say about this

[mununki]

I've tried a rough implementation of heterogeneous array for deps expression:
https://rescript-lang.org/try?version=v10.1.2&code=C4TwDgpgBMA8DkBDAfFAvFJAoLABArgHYBGA9gB4QAmWokUAThAMZRURjpQAiHAFHAD6yAJQAudmBx1okgM5dEDBohCxJyHABsIwNhzlj9YBRgDavMHwCMIgDQ9+AIkRORAXSxA

[mununki]

Oh, there is v11 for playground. I've tried an implementation using untagged variant:
https://rescript-lang.org/try?version=v11.0.0-alpha.5&code=C4TwDgpgBMA8DkBDAfFAvFJAoLABArgHYBGA9gB4QAmWokUAThAMZRURjpQAiHAFHAD6yAJQAudmBwESFarXDRJAZy4AfKLkTK+RdgDMAloWoioAeUIBRAG4QGIAEoRC7BlA0BBBg0Qg+iD5+sJKiOAA2EMBsHMpiMWCqGJa29k4ubliR0SoADPEqXN6+-gDaALoiWVEJygBMBbFFQWW8YHwAjCIANDz8AESI-SKVQA

[mununki]

To summarize, we seem to be at 3 options.

• Bind only one function, useEffect, and document the 'dep.
• Bind two if deps are optional, like useEffect. e.g. useEffectOnEveryRender, useEffect
• Use an untagged variant to make it a bit more type safe.
  All of the above options give us the convenience of not having to use bindings like useEffectN.

[cristianoc]

help finding criteria...

Choosing one of the three options you provided would depend on several criteria from the user's perspective. Here are some important factors to consider:

1. Ease of Discovery: How quickly can a new user or developer find and understand how to use these bindings? In terms of ease of discovery, option 2 might be the best. Naming the functions descriptively (like useEffectOnEveryRender and useEffect) could make them easier to find and understand at a glance.

2. Error Handling and Debugging: How easy is it to understand error messages or to debug when something goes wrong? Option 3 might be superior in this regard, as using an untagged variant can lead to more descriptive compile-time errors, helping to find bugs more quickly.

3. Knowledge Transfer from React: When considering the ease of transferring knowledge from React, option 1 could be preferred. It matches the original React API more closely, which means that developers already familiar with React's useEffect would likely adapt quicker.

4. Understanding Compiler Errors: Option 3, using untagged variants, could be advantageous for understanding compiler errors. It can provide more specific compile-time checks, making errors easier to understand and fix.

5. Learning Curve: Option 1, binding only one function, useEffect, and documenting the dependency, has a relatively low learning curve for those who already know React. But it might confuse beginners due to the optional nature of dependencies. Option 2, binding two separate functions, has a slightly steeper learning curve but can reduce confusion due to explicit naming. Option 3, using an untagged variant, would likely have the steepest learning curve due to introducing the concept of untagged variants.

6. Consistency: Is the API consistent with the rest of your codebase? It is often advantageous to choose an option that is more in line with the coding styles and conventions in your existing codebase.

In conclusion, all these options have their advantages and trade-offs. The ideal choice would depend on the specific needs, preferences, and the level of experience of your team.

[cristianoc]

1. Bind only one function, useEffect, and document the 'dep.
2. Bind two if deps are optional, like useEffect. e.g. useEffectOnEveryRender, useEffect
3. Use an untagged variant

Is 2 the same as 1 with one extra function that one might or might not use?

[mununki]

1. Bind only one function, useEffect, and document the 'dep.
2. Bind two if deps are optional, like useEffect. e.g. useEffectOnEveryRender, useEffect
3. Use an untagged variant

Is 2 the same as 1 with one extra function that one might or might not use?

Indeed. We can rewrite as below:

1. Bind only one function, useEffect, and document the 'dep.
2. Add binding without the second argument, e.g. useEffectOnEveryRender besides useEffect in the first option.
3. Use an untagged variant

[cknitt]

I'd say let's prioritize "Knowledge Transfer from React" and "Learning Curve" and go with 1 or 2.

If you prefer 1, I am fine with it as long as we make it clear in the documentation what the difference between [] and () is and that () is not what you usually want.

[mununki]

Let's pick 2! In my opinion, option 2 is most balanced 😄 . If so, does the name make sense to you? useEffectOnEveryRender?

[cristianoc]

Sounds like it's settled

[cknitt]

Why this format change?

[mununki]

In my editor, vsc, it formats 🤔 The version of extension is v1.18.0

[mununki]

Need to revert it?

[cknitt]

Ah, right, same for me. Not necessary to revert it, but this formatting is a bit weird, I will report it in the compiler repo.

[cknitt]

Ah no, sorry, it is correct that way, as the line above does not have a =>. 🤦

Maybe better to rewrite in one line as | "" | "/" => list{}.