-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Experimental] PPX + dynamic forms support #60
Conversation
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 One of the things that might be improved is the I love what you're doing and if there's anything you can section out and need/want some help on LMK. |
|
@johnhaley81 @baransu Thanks for feedback!
It makes perfect sense! Will change this 👍
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.
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.
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.
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! |
|
6580867
to
a4480c3
Compare
AFAICT all features of non-ppx version are implemented. Changes since the first post:
let validators = {
email: {
strategy: OnSubmit,
validate: ...,
},
remember: None,
};
[%form
type input = {
email: [@field.async] string,
password: string,
asyncFieldTriggeredOnBlur: [@field.async {mode: OnBlur}] string,
};
];
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
} |
87847d1
to
da60cf3
Compare
4980e73
to
68b4b45
Compare
@johnhaley81 Will take a look soon. |
@johnhaley81 Published |
@alexfedoseev the 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. 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 I also liked that |
It would require attribute b/c in AST it would be just 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.
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 |
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 passing
action(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