fix: remove [@mel.uncurry] from React.useCallback* functions

[anmonteiro]

React.useCallback can take a function of multiple arguments. In the following example:

let foo = React.useCallback((_a, _b) => Js.log("HELLO"));

The logging never occurs with [@mel.uncurry] because Melange generates the following code:

function (_a) { return function (_b) { ... }}

We preserve the uncurried callbacks in React.Uncurried.useCallback*

[jchavarri]

In the following example:

let foo = React.useCallback((_a, _b) => Js.log("HELLO"));

Maybe this could be added to the tests.

[jchavarri]

I tried to understand better why this change works, and why it doesn't break any other existing cases. Leaving here a note for the record.

The reason is that uncurry is only necessary if the callback function is called from the JavaScript side. For useCallback, this never happens: React doesn't check or call the function itself, nor the # of arguments it has. In other words, the function will only be called from the OCaml side.

We preserve the uncurried callbacks in React.Uncurried.useCallback*

The above makes me wonder about the Uncurried version. If we know the function will always be called from OCaml side, is there a point to keep the mel.uncurry attribute in that one?

[anmonteiro]

For useCallback, this never happens: React doesn't check or call the function itself, nor the # of arguments it has. In other words, the function will only be called from the OCaml side.

This isn't true. I caught the bug when passing it to JavaScript, e.g.:

module X = {
  [@mel.module "foobar"] [@react.component]
  external make: (~takesFunction: (a, b) => unit) => React.element = "default";
};

module FromOCaml = {
  [@react.component]
  let make = () => {
    let myFn = React.useCallback0((a,b)=>a+b);
    <X takesFunction=myFn />;
  };
};

[jchavarri]

I think in the case you mention, the takesFunction prop should be using @u attribute to uncurry. Otherwise it can be trivially broken. E.g. what happens if you do this?

let myFn = React.useCallback0((a) => {let f = (b) =>a+b; f});
<X takesFunction=myFn />;

I think it breaks even without useCallback, so the problem does not seem related to React.useCallback at all?

[anmonteiro]

That's true. Are you suggesting we remove [@mel.uncurry] from the Uncurried module too?

[anmonteiro]

It's also not possible to use uncurried functions in useCallback. Playground example:

[anmonteiro]

So perhaps the Uncurried module should specify those explicitly? Something like this (better naming suggestions appreciated):

module Uncurried = {
  [@mel.module "react"]
  external useCallback1_0:
    ((. 'input) => 'output, [@mel.as {json|[]|json}] _) =>
    ((. 'input) => 'output) =
    "useCallback";

  [@mel.module "react"]
  external useCallback2_0:
    ((. 'input1, 'input2) => 'output, [@mel.as {json|[]|json}] _) =>
    ((. 'input1, 'input2) => 'output) =
    "useCallback";
};

[jchavarri]

Are you suggesting we remove [@mel.uncurry] from the Uncurried module too?

Not really. With the message above I just wanted to express that the code in the react JS library (which we bind against) will never call the function passed to useCallback. So using uncurry or not in this case does not make any difference as you noted originally :)

I am not sure about the support for uncurried functions... I need to think about it more.

[jchavarri]

I think we could simplify things quite a bit if we get rid of the function type on the useCallback binding. So instead of 'input => 'output we just use 'a.

I know this might sound unsafe, but ReactJS itself doesn't care about whatever is on the value passed to useCallback. And from OCaml side, things will be type safe because whatever gets passed to useCallback comes through the other side. You can potentially call useCallback(5) but you could never "call" it.

[@mel.module "react"]
external useCallback0: ('a, [@mel.as {json|[]|json}] _) => 'a = "useCallback";
let x = useCallback0((. a, b) => a + b);

let y = x(. 2, 3);

Playground.

[anmonteiro]

Seems more correct than the current situation anyway.

[anmonteiro]

@jchavarri used your suggestion. Added a generic 'fn argument to the useCallback functions, and removed them from the Uncurried module, since they'd be the same.

[davesnx]

It's minor but there's a runtime check of being sure that useCallback gets a function. That's probably why the callback type was added in the first place.

[anmonteiro]

It's minor but there's a runtime check of being sure that useCallback gets a function. That's probably why the callback type was added in the first place.

Do you mean in React.js proper? That's even better, then. React can make sure we're passing a function (though it'll be caught at development time).

[jchavarri]

Looks good afaict, hopefully the weaker types don't pose an issue. I was trying to trigger the warning that @davesnx mentioned, but I was not able to.

[jchavarri]

Seems React just swallows whatever is passed to it: https://stackblitz.com/edit/react-playground-practice-13ux64?file=index.js

[davesnx]

@jchavarri I got a TS error on JSX and didn't realised useCallback eats everything.