Support index on array types

[gaku-sei]

Target

The idea of this pr is to generate gadt constructors with an index attribute when the field is of type array.

This:

module StateLenses = [%lenses
  type state = {
    hobbies: array(string),
  }
];

Will generate:

    type _ field =
      | Hobbies: string array field
      | HobbiesAt: int -> string option field
      | HobbiesAtExn: int -> string field

With the above example we can now type:

open StateLenses;

let state = {
  hobbies: [|"foo", "bar"|],
};

state->get(Hobbies); // returns `[|"foo", "bar"|]` like usual
state->get(HobbiesAt(1)); // returns `Some("bar")`
state->get(HobbiesAt(2)); // returns `None`
state->get(HobbiesAtExn(1)); // returns `"bar"`
state->get(HobbiesAtExn(2)); // throws an exception (index out of bound)

This becomes extremely useful once used with the new Field component introduced here. Since one can now write:

<PostAddForm.Field
  field=HobbiesAt(1)
  render={({handleChange, error, value}) =>
    // value has type `option(string)`
  }
/>

Within a loop it can be:

{
  hobbies
    ->Belt.Array.mapWithIndex((index, hobby) ->
      <PostAddForm.Field
        field=HobbiesAtExn(index)
        render={({handleChange, error, value}) =>
          // value has type `string`
        }
      />
    )
    ->React.array
}

Js.Dict

We could also consider adding this feature for Js.Dict.t with a key being a string. Most of the work is already done/prepared.

One word on the getter

Currently, the getter is a bit awkward, since you have to pass an optional value in order to update the field (with *At, not with *AtExn).

state->set(HobbiesAt(1), Some("baz"));
state->set(HobbiesAtExn(1), "baz");

In order to allow the new provided value to be a string here instead of an option(string), we have to make the generated field type change, and accept two arguments:

(* First argument being the getter, second one being the setter *)
type (_, _) field =
  | Hobbies: int -> (string option, string) field

When setting an array's value using *At and passing None, nothing happens:

state->set(HobbiesAt(1), None); // Nothing happens

[gaku-sei]

The {withIndexes} option is not yet implemented, but the rest seems to be working as expected.

[fakenickels]

Oh this is very good, thanks a lot. I will review with care soon.

[gaku-sei]

Thank you very much @fakenickels. Notice that it's very wip and some things should probably change (like the way the array is updated in the setter).

While writing the description I also wondered if having the constructor with index alongside the "global" one wouldn't be better (it would also make the withIndexes option useless):

type _ field =
  | Hobbies : string array field (* like usual *)
  | HobbiesAt : int -> string option field

We could even have an unsafe *AtExn for when the Field component is used in a map loop (which is an extremely common case):

type _ field =
  | Hobbies : string array field (* like usual *)
  | HobbiesAt : int -> string option field
  | HobbiesAtExn : int -> string field

[gaku-sei]

Small update: I implemented the above, and dropped the ppx_tools from the esy file for now (probably better to add it in a different PR).

It should be more or less ready for review, we just need to solve two issues:
1 - The generated field type could take two args instead of one. So that the first one would represent the getter, and the second one the setter
2 - How should we update the array in the setter? I'm using the xs.(index) <- value syntax for now since it's the fastest, but it's not immutable anymore.

[fakenickels]

Sorry about taking so long to give feedback, but I promise I'll be taking a look til the end of this week

[fakenickels]

now with this additional code we reached the column size of text, I think it's a good idea to break this down into smaller functions

[gaku-sei]

Sorry for the late reply, and thank you very much for your review @fakenickels

When implementing this solution I was also considering some "refactor" but didn't want to change too many things. Also because I don't know what the plans are to make the code base scales.

Do you have any ideas on how this could be refactored? Should I just split the code into functions, or is it ok to use the [%expr ...] annotation to make the code a bit less verbose?

[fakenickels]

Hey there @gaku-sei !
I agree 100% with you, the code is really verbose currently and we could make more usage of Ast helpers from OCaml. About using [%expr], i'm in for it!

Just to let you know I had to change a little bit the code to make it compatible with the new BS

[fakenickels]

@gaku-sei sorry about taking so long to get updates here. I'm willing to merge this already, but could we split the code in more functions or use Ast_helper or ppx_metaquot to make it less verbose like you suggested?