Coerce form values to Prisma type

[Tobbe]

No description provided.

[cannikin]

OMG this is amazing, does it really work? Have you solved just about all outstanding issues with scaffold generation of non-string types??

[cannikin]

How come this is the only one with an explicit type prop set and not also Int?

[Tobbe]

When deciding what coercion to make, I look at two things; the name of the component used for the field, and the type prop. For Int I use a <NumberField> component, and those will be coerced using parseInt. For Float I use a regular <TextField>, for which no coercion is done at all by default. By adding type: 'Float' the parseFloat coercion will kick in.

Now, you might wonder why I use <TextField> for floats, and the reason for that is that <NumberField> is just a <input type="number"> under the hood, and those don't support decimals by default. There is a way around that, by setting the step attribute to something < 1 (and > 0). For example step="0.01" will allow you to enter floats with two decimals, but not three or more decimals. I didn't want to enforce how many decimals our scaffolded forms support, so that's why I went with <TextField>. That way the user can enter however many decimals they want.

It might be a little misleading to have type be part of validation since I'm not actually using it for form validation. Should I move it to its own prop? And if so, maybe rename it to something like dataType to not confuse it with the type used on the underlying <input>

Another option might be to actually use it for validation by trying to build custom validation functions to pass to react-hook-form based on the type. If we want to go this route, could we do that as a separate PR?

[cannikin]

Makes sense! I like that idea of changing it to dataType so as not to confuse with regular type on the input. And it should probably move out of validation to keep it simple and we can pass that object in bulk to react-hook-form without having to tweak anything.

Yeah I think in the future we want to let users set dataType themselves on any field they want, including passing functions for custom coercion. We can definitely do that as another PR.

[cannikin]

Love it! Ready to merge or did you want to add any more tweaks?

[Tobbe]

I'd like to test the Json stuff, but ran into some problems, see https://community.redwoodjs.com/t/error-unknown-type-json-did-you-mean-json/817/6 Would also like to add some kind of automated tests, but not really sure how to do it. Any ideas?

…

On Thu, Jun 25, 2020, 18:30 Rob Cameron ***@***.***> wrote: Love it! Ready to merge or did you want to add any more tweaks? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#749 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAHQSKO74KILFZRSZINOILRYN3TPANCNFSM4OHFHJPQ> .

[cannikin]

Would also like to add some kind of automated tests, but not really sure
how to do it. Any ideas?

Well, the extent of the generator testing is to create fixtures and compare with the output, but now all the work happens in that function so there won't be any code differences in the scaffold.

Did you want to try and add tests to form.js itself? I'd love to have some, I just didn't know how to go about testing that...it needs to run in the context of a React app, and have context and providers... ugh. It might be time to call in the big guns: @RobertBroersma any idea how we can go about testing the form.js components? :)

[RobertBroersma]

Hey @cannikin and @Tobbe This looks very cool!

If I'm not mistaken form.js is all about user-facing components; the form and form fields. So to test it I would stay as close to those users as possible, which is what @testing-library is all about!

To get the contexts and providers you need you can render using the @redwoodjs/testing package. This will wrap whatever you render in <RedwoodProvider />. This will actually be great feedback for the testing package as I/we haven't really given a lot of thought to forms yet.

For the actual tests I would:

1. Create a dummy component that implements the form and form fields. Pass in a Jest mock to the form submit handler.
2. Type some stuff in the fields.
3. Click the submit button.
4. Assert that your submit mock was called with the coerced values.

Note that the @redwoodjs/testing package is still in the process of being documented, so feel free to message me if anything is unclear on that end. In the end it's mostly @testing-library/react that you will need!

[Tobbe]

I haven't forgotten about this one, and now that #514 needs it too, I'll make sure to do more work on it.

I'm pretty confident I can get some tests working, but I'm still not sure about the JSON stuff mentioned earlier. Anyone else have any thoughts on that?

[brianespinosa]

@Tobbe this is awesome. I ran into some of these issues you resolve here over the weekend too when I noticed what was happening in #514. I can start working on a PR for that and hold off for whatever you have here. If I get that PR up, technically it would still work minus the coercion on POST for a date field.

[Tobbe]

@brianespinosa You could also base your work off of my branch, https://github.com/Tobbe/redwood/tree/tobbe-coerce-form-values and get it all working in one go. And then just rebase it on top of https://github.com/redwoodjs/redwood when this PR is merged. That way you can get all functionality in from the start, and you won't have any merge conflicts :) But, of course, do what works best for you 🙂

[brianespinosa]

@Tobbe duh to me. Yes, I will do that. ;)

[Tobbe]

Please merge #800 first, so that we can see the tests I have added here passing when run by GitHub Actions as well, before merging this

[Tobbe]

@RobertBroersma Please take a look at the tests. Do they look good to you? Anything you want me to change?

[RobertBroersma]

Looks good to me!

[Tobbe]

I'm happy with this PR now

@cannikin Please merge if you're happy with the way everything looks

[cannikin]

Thanks so much for this one @Tobbe!