Closed alex35mil closed 4 years ago
This looks great IMO! We're big on domain driven design and reformality is one of the things we use to be a gatekeeper for our data.
The way we use our forms is the same as the way you have written it out above. At the input level everything is a string
, bool
and we try to parse the data onto the types that we're expecting.
One of the things that might be improved is the update<fieldName>
function. If that didn't need to take in the previous input state then you would reduce the complexity of the update since you could clobber over a previously updated value if you are sending in a stale input state.
I love what you're doing and if there's anything you can section out and need/want some help on LMK.
update<fieldName>
function as setState
where it receives state => state
function instead of state
.@form.collection
is ppx intended to support? Is it only an array
or other types like list
or Belt.Map.String
could be possible?[%form]
. This is harder if you're type is more complex and you're wrapping it into a module to hide implementation details. @johnhaley81 @baransu Thanks for feedback!
One of the things that might be improved is the update
function. If that didn't need to take in the previous input state then you would reduce the complexity of the update since you could clobber over a previously updated value if you are sending in a stale input state. As @johnhaley81 pointed - it would be great to treat update function as setState where it receives state => state function instead of state.
It makes perfect sense! Will change this π
What collections does @form.collection is ppx intended to support? Is it only an array or other types like list or Belt.Map.String could be possible?
The rules here are the same as for non-collection fields except it must be array of records. I need type information for each field in collection the same way as for the root fields to keep track of the state of each field in collection. I use type name inside an array for looking up the record type definition inside a list of items in the config module.
Types have to be inlined in [%form]. This is harder if you're type is more complex and you're wrapping it into a module to hide implementation details.
Yeah, this is unfortunate requirement that I can't overcome yet since I need names of the fields and type information. With ppx, I can get this only when these things are inlined.
I'm using the forked version on reformality (mostly described here: #43 (comment)). My use-case is hardcore - it's used to handle complex graph with few levels of nesting. For most of the cases, proposed ppx is a great solution but it would be great to opt-out of it and create more complex cases by hand.
It's totally possible since ppx generates c/p'able code. In this scenario, it acts more like scaffolder of the base implementation that can be updated by hand.
if there's anything you can section out and need/want some help on LMK
That's a bit hard in the current state since I'm doing changes all over the place while reimplementing existing functionality. I'll definitely ping you once there will be pieces that can be implemented in parallel!
[@field.deps]
is in.
AFAICT all features of non-ppx version are implemented.
Changes since the first post:
validators
definition is moved to config. It allowed a number of optimizations + it's possible to unwrap validators, e.g. if validator exists just define it. If it doesn't, set it to None
.let validators = {
email: {
strategy: OnSubmit,
validate: ...,
},
remember: None,
};
[%form
type input = {
email: [@field.async] string,
password: string,
asyncFieldTriggeredOnBlur: [@field.async {mode: OnBlur}] string,
};
];
output
type of the value that is returned from sync validator, this way it's guaranteed on a type level that async validator wouldn't be called unless sync validator returned Ok(x)
. I want to get feedback on this, e.g. would anyone need input
record or single value should be enough (I assume it should but).validateAsync: 'outputValue => Js.Promise.t(result('outputValue, 'message))
type asyncResult('outputValue, 'message) =
| Validating('outputValue)
| Result(result('outputValue, 'message));
switch (form.emailResult()) {
| Some(Validating(_)) => <Spinner />
| Some(Result(Ok(_))) => ...
| Some(Result(Error(error))) => error->React.string
| None => React.null
}
With [@field.collection]
implemented, I think things are pretty much ready for the first beta.
I also added skeletons of 2 types of test suits:
Not much of tests are actually written though.
I imagine the plan would be:
--
@johnhaley81 @baransu I updated signature of updateField()
functions so those take input => input
argument but spotted very unfortunate issue. This code would produce runtime error:
<input
value={form.input.name}
onBlur={_ => form.blurName()}
onChange={event => {
form.updateName(_input => {name: event->ReactEvent.Form.target##value});
}}
/>
It happens due to synthetic events pooling. Since callback gets triggered asynchronously, by the time it gets called, event is already null'ed. To fix this, it should be written this way:
<input
value={form.input.name}
onBlur={_ => form.blurName()}
onChange={event => {
let value = event->ReactEvent.Form.target##value;
form.updateName(_input => {name: value});
}}
/>
Which seems very subtile. What are your thoughts on this?
@johnhaley81 Pinging you on RC.
I'm almost finished with the docs (rest few will be finished tomorrow): https://github.com/MinimaHQ/re-formality/tree/ppx/docs
And published RC: re-formality@next
Thanks @alexfedoseev I'm going to pull it down now and test it out!
@alexfedoseev would it be possible at all to have different fields in input
and output
?
For example say we want to make a date/time picker with fields:
type input = {
minutes: int,
hours: int,
day: int,
month: int,
year: int
};
We'd really want an output of a Js.Date.t
and not all of these fields.
type output = Js.Date.t;
Would that be a possible change?
EDIT:
I see now that the validators are mapping straight across from the fields in input
to the fields in output
so it doesn't look like the above is possible without a change in how the ppx works.
@johnhaley81 Yeah, for now it's not possible to combine multiple fields into single type. But I thought about [@field.entity]
(or something like this) which suppose to handle this kind of thing. Though I didn't look into details of it yet.
I keep thinking of things as I'm going through it to only get to the solution later in the docs ππ
I'll save my comments then till the end and then I'll do a write up.
Really quick prototype of [@field.entity]
:
type input = {date: [@field.entity] date'}
and date' = {
minutes: int,
hours: int,
day: int,
month: int,
year: int,
};
type output = {date: Js.Date.t};
type validators = {
date: {
entity: date' => result(Js.Date.t, message),
fields: {
minutes: {
strategy: Strategy.t,
validate: input => result(int, message),
},
...
}
}
};
// Interface
form.updateDateMinutes(1)
form.dateMinutesResult
form.dateResult
@alexfedoseev I'm getting a type error while trying to define submissionError
.
@johnhaley81 Will take a look soon.
@johnhaley81 Published 4.0.0-beta.2
with the fix.
@alexfedoseev the submissionError
fix works great!
A lot of the forms are cleaner now and much easier to understand what's going on. This is a great change!
One of the things that I was thinking about was with with the setters (i.e. update<fieldName>
)
With the runtime exception issue, maybe it's better to have the function that passes in the state to be a different one? So the default setter could be update<fieldName>
which just takes in the value and blindly plops it into the state and a more advanced one update<fieldName>WithState
would give you copy of the latest state if you needed it.
I also liked that entity
change. Would it need that [@field.entity]
tag or would the ppx be able to derive it since the field is a record?
I also liked that entity change. Would it need that [@field.entity] tag or would the ppx be able to derive it since the field is a record?
It would require attribute b/c in AST it would be just Ptyp_constr
and it would require parsing of the whole structure for each field to find out if it's record or not. So it's easier and faster to be explicit here.
I will create separate issue to discuss details since I have one more use-case for record field in mind and I'm not sure yet if it would be useful and if it would how to split 2 these attributes.
With the runtime exception issue, maybe it's better to have the function that passes in the state to be a different one? So the default setter could be update
which just takes in the value and blindly plops it into the state and a more advanced one update WithState would give you copy of the latest state if you needed it.
Also tend to move it to separate issue. In general, the less api the better and both of ways have own caveats, so instead of one issue it would probably give us two and more things to keep in mind for users.
I extracted all leftovers to own issues. Looks like ready to merge!
Merging. I think it makes sense to keep it on @next
tag for sometime and add some integration tests. Then publish as latest
.
This PR implements PPX that introduces
input
/output
concept. It handles the most basic use-case (see example), for now. But might be extended to support all features of current non-ppx implementation + dynamic forms.Config shape:
input
holds data that are shown in UI. Usually, these arestring
s orbool
s.output
is what you get insubmit
handler if form is fully valid.Both
input
andoutput
types must be records with the same set of fields (definitions must be inlined). In the simplest possible case, wheninput
is the same asoutput
, the latter can be alias of the former:Validators are defined as an argument of
useForm
hook which gets generated by ppx. A type signature of validators is changed: instead oflist
of validators it's a record, with same fields asinput
/output
types. Validator function must return value of field type inoutput
record (instead ofValid | NoValue
variant) wrapped inresult
. Basically, it's guaranteed on type level that you'd be able to submit a form only when all validators returnedOk(outputValue)
.Use cases:
input
/output
are the sameinput
/output
are differentmessage
&submissionError
typesCollections, dependent fields & async forms are not implemented yet so examples below are just drafts that might be changed.
The main downside of this approach I see is that more code gets generated. It can be optimized in some ways but still. Also, source code became more complex.
Why
field
type is gone? Initially, I prototyped ppx output using field type + GADTs but in the end I bumped into type issue when I can't preserve type information while passingaction(i, o)
type toReact.useReducer
. I could manage this part to compile only by hiding GADT types with existential wrapper but it wouldn't work since this information is required b/c validators returnresult('output, 'message)
type which is field specific (output
bit).The code below results in the following error:
If it's possible to solve this case, it would probably help to reduce amount of generated code. On the other hand, I'm not sure that code size difference would make a huge difference and dumb set of functions is easier to manage (e.g. debugging/error messages).
Anyway, appreciate feedback on this experiment.
/cc @baransu @mlms13