itemPropsOptions index shouldn't be an option

[kgoggin]

Hi! First off, thanks a ton for writing this. I've been using it to implement a dropdown in ReasonML and it's been great - I've actually learned a ton about writing bindings from looking at the code!

I discovered an issue with the way itemPropsOptions is defined. As it is, index is defined as option(int). When this gets compiled to JS, the result is that the index gets wrapped in an array, and that's what gets passed to the JS Downshift method getItemProps. Downshift itself doesn't type check this value at all, it just trusts that its an int, and uses it as such. When that item gets hovered, the internal state is updated with that item's index... which now gets set as an array containing an int instead of just an int.

Then, highlightedIndex is typed as a Js.Nullable.t(int). Since that's not compatible with the actual highlightedIndex that's getting passed in from Downshift, you can't actually ever check to see if the current item's index matches highlightedIndex.

I can think of a couple of different ways to fix this, which I'll list below. I'm happy to make the change and submit a PR, but I wanted to run the approach past you first!

Option 1: Js.Nullable

One option would be to change the type to be as follows:

type itemPropsOptions = {
  .
  "index": Js.Nullable.t(int),
  "item": any
};

This ensures we're actually passing an int to Downshift, and keeps the sprit of the option without changing too much. I've made this change on my own and confirmed it works with my code.

Option 2: Pass options as labeled arguments

This would break with the way you're exposing the external methods directly, but you could write a new method for getItemProps like this:

let getItemProps = (t, ~item:any, ~index=?, ()) : any => {
    let itemPropsOpt = {
      "item": item,
      "index": Js.Nullable.from_opt(index)
    };
    externalGetItemProps(t, itemPropsOpt);
  };

The advantage of that approach is it feels more in the spirit of the actual API where index is optional. You'd also need to make the same change as in Option 1 to reflect that index was potentially null.

Let me know your thoughts on how best to proceed and, like I said, I'd be happy to submit a PR with the preferred approach! Thanks again!

[emmenko]

Hey, thanks a lot for looking into this! Glad it’s helping you with getting into the bindings.

To be honest I haven’t checked/battle-tested all the options, so definitely there might be some little mistakes and there is room for improvements.

Regarding the issue, you’re totally right. Between the options I would say the second option is more “reason-y” and I think it’s more preferable.
In general the usage of the bindings does not need to reflect 100% the API of the JS library. In those cases the bindings can hide some implementation details to make the bindings compatible.

So yeah, if you can provide a PR it would be awesome, otherwise I’ll have a look when I’m back next week 😉

Thanks! 🙏