Use 'union trick' to model hook dep arrays

[yawaramin]

Using BuckleScript 7.1.0's union type
trick, we can
now model hook dependency arrays as (almost) heterogeneous just like in
JavaScript. We now need only the following two bindings:

• useEffect to model not passing in a dependency array
• useEffectN to model passing in an array of zero or more dependencies

The same technique applies to the rest of the hook bindings, and the
unneeded bindings useFoo0 to useFoo7 are now deprecated.

[yawaramin]

To be clear: we don't actually need this dep type in the ReasonReact binding. All we need is the hooks that take dependencies to take an array('a) or array(someType). The point is that it's an array of a fixed type, where the array would be empty e.g. [||], contain a single element e.g. [|somDep|], or could contain elements wrapped in some unboxed GADT like dep. I just chose to make it array(dep) to provide an out-of-the-box solution and lock it in. If e.g. BuckleScript shipped with this unboxed GADT e.g. [@unboxed] type any = Any(_): any, we could use that.

[jfrolich]

The advantage is that it removes the separate functions for all the tuple sizes. The disadvantage is that we have to wrap each dependency in a GATD. I am not sure if this is a net gain because:

• People are already familiar with the trailing number for other API's, so it fits in with how other libraries work. (Such as within Js.logX etc).

• The GATD is not an implementation detail, it is exposed to the user. GATD are quite an advanced language feature, so I am afraid that it might confuse people (why can I wrap this ATD around different types?). They don't have to understand it to use it, but I think there are advantages when the surface API only contains well-known language features (with exceptions of course).

If the Any GATD becomes an idiomatic practice for Bucklescript this makes a difference I think (not sure what the plans are when it comes to for instance the BS standard library).

[yawaramin]

@jfrolich about the GADT–you have a point. It's actually overkill. All we really need is a function React.dep: 'a => dep which casts dependencies into a single type for the dependency array. So e.g.:

// React.re

/** A React hook dependency. */
type dep;
external dep: _ => dep = "%identity";

[@bs.module "react"]
external useEffectN: (..., array(dep)) => ... = "useEffect";

Now, no need for advanced type-level features. React already uses the %identity technique to type-cast various types into React elements, this could be explained in a very similar way.

About the familiarity with different APIs–I think yes and no. Some bindings are in that style but I think it usually irks people. And judging by the Discord conversation that triggered this PR, people coming to Reason may really question the lack of ergonomics of having N different functions where they needed only one in JavaScript. But on the other hand they may also question the ergonomics of having to wrap the dependencies. So maybe it balances out.

[jfrolich]

@jfrolich about the GADT–you have a point. It's actually overkill. All we really need is a function React.dep: 'a => dep which casts dependencies into a single type for the dependency array. So e.g.:

// React.re

/** A React hook dependency. */
type dep;
external dep: _ => dep = "%identity";

[@bs.module "react"]
external useEffectN: (..., array(dep)) => ... = "useEffect";

Now, no need for advanced type-level features. React already uses the %identity technique to type-cast various types into React elements, this could be explained in a very similar way.

About the familiarity with different APIs–I think yes and no. Some bindings are in that style but I think it usually irks people. And judging by the Discord conversation that triggered this PR, people coming to Reason may really question the lack of ergonomics of having N different functions where they needed only one in JavaScript. But on the other hand they may also question the ergonomics of having to wrap the dependencies. So maybe it balances out.

Ah cool, yes I think a dep function would be better for sure! This removes my concern because now we hide the more advanced implementation to the user! And familiarity not a problem anymore because this pattern is similar in React itself (casting toReact.element).

(With this the unboxed type is not even necessary anymore right?)

[yawaramin]

Yes indeed, now the type dep is a standard abstract type with no implementation detail. At runtime it will be whatever JavaScript value is passed in.

[sikanhe]

is this really better? key stroke wise we actually gained a alot

[yawaramin]

I think for me the main benefit is not having to remember the rules of how to map the various bindings into the way they'll behave in the JavaScript. Current situation:

• useEffect will not pass any dependency array
• useEffect0 will pass in an empty array
• useEffect1 will pass in an array so I can use it to pass in zero dependencies (which I can also use useEffect0 for) or I can use it to pass in one dependency (which is the intention) or I can use it to pass in many dependencies of the same type, but if people come across that in a codebase it will be confusing because it's useEffect1, so why are there many deps?
• useEffect1 to 7 will pass in exactly those number of deps, which is not confusing, but I still have to remember to pick the correct function depending on the number of deps, which is kinda annoying, and I'll have to roll my own solution if I need more deps

Proposed situation:

• useEffect will not pass in a dep array
• useEffectN will pass in a dep array, but I need to cast each element in the array with the React.dep function. Of course if there are no elements then it's just an empty array and no React.dep required

[sikanhe]

I was thinking the other day that i don't mind giving up type safety here and let me pass 'a to the dep array argument (i would just use a tuple always)

[sapristi]

For me the main issues with the current implementation are

• it feels like a hack
• It's troublesome to rewrite some code from useEffect1 to useEffect2

Proposed situation:

* `useEffect` will not pass in a dep array

* `useEffectN` will pass in a dep array, but I need to cast each element in the array with the `React.dep` function. Of course if there are no elements then it's just an empty array and no `React.dep` required

That seems "cleaner" than the GADT, in that you don't need the 1..N suffixes/GADT constructs.

I was thinking the other day that i don't mind giving up type safety here and let me pass 'a to the dep array argument (i would just use a tuple always)

AFAIK there is no loss of type safety here.

Edit:

There is indeed type safety loss, namely that the length of the supplied array could change between calls. This makes me realize that a tuple-based solution is indeed the most canonical one with respect to the type expected by useEffect. So I think finally a GADT based solution could be "better".

[tsnobip]

What about using a diff list? Probably overkill too, but for the sake of bikeshedding ^^

[jfrolich]

What about using a diff list? Probably overkill too, but for the sake of bikeshedding ^^

Can you elaborate?

[cem2ran]

Possible @tsnobip. Question is if you want to incur the runtime conversion cost, which may or may not be a real factor. I'm not sure. Here's a quick attempt @jfrolich:

type dep;

/** [dep(value)] safely type-casts any [value] into a React hook dependency. */
external dep: _ => dep = "%identity";

[@bs.module "react"]
external _useEffectN:
  ([@bs.uncurry] (unit => option(unit => unit)), array(dep)) => unit =
  "useEffect";

module Deps = {
  type hlist(_) =
    | []: hlist('a)
    | ::(('a, hlist('b))): hlist('a);

  // Fold the tuple (nested array representation) into a single dimensional array. (naive approach. tailrec in real world)
  let rec array_of_hlist: type a. hlist(a) => array(dep) =
    fun
    | [] => [||]
    | [hd, ...tail] => Array.append([|hd->dep|], array_of_hlist(tail));
};

let useEffect = (effects, deps: Deps.hlist('deps)) =>
  _useEffectN(effects, deps->Deps.array_of_hlist);

// Usage:

useEffect(
  () => {
    Js.log(["bob", true, 123]->Deps.array_of_hlist);
    // Outputs: ["bob", true, 123]
    None;
  },
  ["bob", true, 123], // surprising to me, is that `Deps.[]` is not required here in the scope! TIL
);

[tsnobip]

Thanks for the example @cem2ran, I was trying to write one but the reasonml playground doesn't like GADTs on lists, I don't know why. @jfrolich that example is indeed what I meant.

I think it looks pretty neat especially since you don't have to open the diff list module to use it. The additional runtime cost is O(n) with n being very small anyway, so I think it's quite acceptable.

[jfrolich]

Is there a way to make Bucklescript transform the recursive function calls into a loop? Or Obj.magic into a list of deps and then Array.fromList?

[Et7f3]

Is there a way to make Bucklescript transform the recursive function calls into a loop

tail rec function are already transformed in loop.

Last bucklescript don't seem to handle inline attribute or don't use %private for inline the function calls.

[@bs.inline]
let%private map = (f, l) => {
  let rec map = acc =>
    fun
    | [] => List.rev(acc)
    | [e, ...l] => map([f(e), ...acc], l);
   map([], l);
};

let a = map(a => a + 1, []);

generate:

// Generated by BUCKLESCRIPT, PLEASE EDIT WITH CARE
'use strict';

var List = require("bs-platform/lib/js/list.js");
var Curry = require("bs-platform/lib/js/curry.js");

function map(f, l) {
  var _acc = /* [] */0;
  var _param = l;
  while(true) {
    var param = _param;
    var acc = _acc;
    if (!param) {
      return List.rev(acc);
    }
    _param = param[1];
    _acc = /* :: */[
      Curry._1(f, param[0]),
      acc
    ];
    continue ;
  };
}

var a = map((function (a) {
        return a + 1 | 0;
      }), /* [] */0);

exports.a = a;
/* a Not a pure module */

Even with map defined as local function don't help the inlining :(

[tsnobip]

yes I guess you could write array_of_hlist this way to make sure it's O(n) :

external toDepList : hlist(_) => list(dep) = "%identity";

let array_of_hlist: type a. hlist(a) => array(dep) =
  hlist => hlist->toDepList->Belt.List.toArray;

[cem2ran]

@tsnobip that wouldn't work due to the underlying representation of the hlist being nested tuples (nested arrays in JS). Maybe this is okay w.r.t React Hooks dependencies? and if so you could just use the "%identity" trick to pass along the dependencies "unflattened". I don't think you can get around the O(n) if there's conversion involved, which again is probably not worth discussing when N < 10 in most cases. Maybe the JS engine has a perf trick to flatten nested arrays, but I think this is as good as it gets unless you're willing to do PPX stuff.

[tsnobip]

@cem2ran , sorry I didn't pay attention enough, the diff list implementation I was thinking about is this:

type dep;

module DiffList = {
  type t('ty, 'v) =
    | []: t('v, 'v)
    | ::('a, t('ty, 'v)): t('a => 'ty, 'v);

  external toDepList : t(_) => list(dep) = "%identity";
  let toArray: type a b. t(a, b) => array(dep) =
    l => l->toDepList->Belt.List.toArray;
};

With this implementation you wouldn't have nested tuples.

[cem2ran]

@tsnobip 👍 I like this encoding better, but fundamentally you're still doing a conversion behind the scenes. Updated impl:

type dep;

/** [dep(value)] safely type-casts any [value] into a React hook dependency. */
external dep: _ => dep = "%identity";

[@bs.module "react"]
external useEffectN:
  ([@bs.uncurry] (unit => option(unit => unit)), array(dep)) => unit =
  "useEffect";

module DiffList = {
  type t('ty, 'v) =
    | []: t('v, 'v)
    | ::('a, t('ty, 'v)): t('a => 'ty, 'v);

  external toDepList: t(_) => list(dep) = "%identity";
  let toArray: type a b. t(a, b) => array(dep) =
    l => l->toDepList->Belt.List.toArray;
};

let useEffect = (effects, deps) =>
  useEffectN(effects, deps->DiffList.toArray);

// Usage:
useEffect(
  () => {
    Js.log(["bob", true, 123]->DiffList.toArray);
    None;
  },
  ["bob", true, 123],
);

[tsnobip]

oh sure, you still need to flatten it to an array, but with a single-digit n, it's far from being a perf issue I guess! I like how it allows to have exactly the same API as JS.

[cem2ran]

@yawaramin pointed out on Discord that we're creating a new array when doing toArray thus causing equality to never be the same, leading to a re-render with no dependency changes? This was also discussed on #reason-dojo. I went down this track like a year ago and now I realize why I may have stopped: https://sketch.sh/s/tt4eMTK6V1GeZOmzSduKTD/

Edit: React does physical equality check of array elements so disregard comment above.

[johnridesabike]

Thinking out loud:

What if we had multiple dep functions for different tuples? Like:

/* React.re */
type dep;
external dep2: (('a, 'b)) => dep = "%identity";
external dep3: (('a, 'b, 'c)) => dep = "%identity";
/* etc... */

/* App.re */
React.useEffectN(
  callback,
  (a, b, c)->React.dep3
);

It's basically the same principle as what we currently do, but it moves the arity from the useEffect function to the dep function. I think this would make cleaner diffs and feel more organized. You don't have to keep syncing your useEffect function with the number of dependencies it has.

Also, would it make sense to rename useEffectN to something like useEffectWithDeps? It's more keystrokes, but much more descriptive. refmt usually puts React.useEffect* on its own line anyway so it wouldn't add more lines of code.

We could possibly even shrink useEffect to one function like this:

/* React.re */
type dep;
external dep: _ => dep = "%identity";
let noDeps = dep(None);

/* App.re */
React.useEffect(
  callbackForEveryRender,
  React.noDeps
);

I haven't tested that, but in JS undefined/None is usually the same as excluding an argument.

[yawaramin]

I tried to keep the changes in the existing functions as minimal as possible, but if renaming the functions more directly, I would probably prefer:

• useEffect: this would take a dep array
• useEffectAlways: this would not take a dep array, therefore it would run on every render

[tsnobip]

yes that would make it much clearer than useEffect and useEffect0 (!?) that have the same signature, almost the same name and opposite effects

[rickyvetter]

Wow lots of discussion here - thanks for all of the input! After reading through discord and here, the main takeaways that I'm seeing are:

1. DiffList impl looks and acts the most like JS which could be benneficial to JS newcomers
2. All of these impls use advanced Reason features (a) that could be confusing to newcomers
3. DiffList is the only proposal that would require a runtime

Something that hasn't been mentioned so far is that hooks deps is not a good api for Reason. We cannot model the rules correctly and we can likely do much much better. If we introduce a runtime dependency then I think it has to be objectively worth the cost. In my mind there are two ways a proposal can prove that it's worth a runtime and a large breaking change:

1. Remove the need for folks to track dependencies at all. This is ideal end state (in ReactJS too) and we have tons of info. I think exploring options here will get us the best possible results and obviously justify a (very small) runtime - if it'd even be required.
2. Provide type errors if folks aren't tracking dependencies correctly. Basically "rules of hooks" for Reason. I don't really think this avenue is as promising because it's hard and the end state isn't great - folks still have to write all of these dependencies!

That said, I agree that it's unclear exactly how confusing/clear any of these proposals would be to newcomers. It is possible that everyone loves DiffList and there is no confusion whatsoever. I think one approach for getting feedback might be to publish reason-react@rfcs (name pending) which could add/remove apis without fear of breaking changes or mistakes. It could ship RR itself along with a very small demo app and we could point to it as a way for newcomers to contribute. "We'd love feedback on rfcs#a" or something like that? As I type this up it sounds complex and a process that might itself confuse newcomers. But I've already come this far, so submitting anyway.

(a) - %identity is an advanced feature, even if we like to pretend that it isn't. React.string and co are by far the most questioned part of the RR api.

[cem2ran]

@rickyvetter Thanks for summary. Not having to track dependencies manually sounds absolutely like the right way to go! Great idea with RFCs too.

[yawaramin]

I have some input/questions about modelling the 'rules of hooks' in Reason, but I'll put those in #465 ... TL;DR: I'm sure people have already thought about this, but what is preventing us from modelling a series of hook calls as monadic binds.

I do like the idea of drafting up alternative implementations and A/B testing them.

[sgny]

This boils down to deciding between matching idiomatic JS API in Reason API or output. I don't really have any issues with the current API either.

Nested JS arrays seem to work fine as argument to useEffect, so heterogenous lists need not incur runtime costs - simply don't flatten at all. There's probably a trivial performance hit when checking a nested array instead of a flat array for changes, but that should be less than flattening the array before checking. It's pretty amazing how little impact a heterogenous list has on use, users may not even realize they are passing a pretty special object as argument.

On the other hand, @johnridesabike's proposal (dep2, dep3, etc.) would allow more idiomatic JS output, while addressing @rickyvetter's point about helping users track number of dependencies. It's a lot easier to understand you've messed up the number of dependencies with such a function.

Of course, both approaches could be allowed as below, giving users the choice that better suits their taste.

module DiffList = {
  type t('ty, 'v) =
    | []: t('v, 'v)
    | ::('a, t('ty, 'v)): t('a => 'ty, 'v);
};

type deps('a) = DiffList.t('a, unit);

external dep2: (('a, 'b)) => deps(_) = "%identity";
external dep3: (('a, 'b, 'c)) => deps(_) = "%identity";

[@bs.module "react"]
external useEffectN:
  ([@bs.uncurry] (unit => option(unit => unit)), deps(_)) => unit =
  "useEffect";

No need to include toDeplist and toArray in DiffList; type deps('a) might be included to make the signature of useEffect less intimidating.

[Et7f3]

Nested JS arrays seem to work fine as argument to useEffect

@sgny No see the code when clicking the third button the effect shouldn't be executed but it is.
The reason is I use [c1, [c2]] if I transform to non-nested the effect isn't executed anymore: [c1, c2]
So your solution isn't applicable because you will introduce unfixable bug in the user code.
https://codesandbox.io/s/twilight-paper-yvvw9?file=/src/App.js

[sgny]

@Et7f3 Thanks for the clear example, I realize my test was incomplete.

[johnridesabike]

If we want to start adding extra runtime to the hooks, would it make sense to namespace them separately under ReasonReact, like ReasonReact.useState etc? They could build off of the zero-cost React bindings.

I think adding extra runtime to hooks can be justified if the runtime increases safety. The problem with hooks currently isn't just that the API is awkward, it's that they're not safe. ReactJS sort-of-solves the safety problem with custom ESLint rules. Reason seems like it's in a great position to enforce correct usage at compile-time in ways that wouldn't be possible in JS.

[yawaramin]

@johnridesabike this is a very good idea imho, use the ReasonReact namespace for non-zero-cost helpers.

[Et7f3]

If we use the non-zero cost approch @johnridesabike we could use the method of brisk reconciler (hook take and return hlist) to ensure type safety.

It also has a ppx support %hook to hide it.

[rickyvetter]

Hey folks. Thanks again for all of the input. I've thought about this a lot and right now isn't the time to make these changes.

I think right now I want to prioritize getting to the goal of 1. matching the React api as closely as possible while eliminating runtime and 2. deprecation of any existing apis that don't fit that model. useEffect1-7 fit this model well as a 0-runtime solution that only uses very simple Reason concepts. It will serve until that release certainly.

Once we get to that point (tentatively referred to as 1.0), I think this discussion should be re-opened. Maybe supporting runtime-dependent features that provide extra coverage is beneficial, but I don't think they should come as part as 1.0.

In the mean time, I would be excited to see experiments and to hear results of using any of these strategies (or of strategies for eliminating the tracking entirely). If you are interested in trying out some of the above and are in a place where you can assume the risk of adopting non-mainstream hooks bindings, please report back.

We'll see you all again in a couple months :)