lenses-ppx
lenses-ppx copied to clipboard
Support index on array types
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
~The {withIndexes}
option is not yet implemented, but the rest seems to be working as expected.~
Oh this is very good, thanks a lot. I will review with care soon.
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
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.
Sorry about taking so long to give feedback, but I promise I'll be taking a look til the end of this week
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?
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
@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?