Error when submitting scaffolded form with Int field

Tobbe commented 4 years ago

When defining a model with an Int field, and then running yarn rw scaffold <name> the generated form doesn't properly handle that Int field. See screenshot below

Would you be interested in a PR that updates the scaffolding template to look something like this?

const ${singularPascalName}Form = (props) => {
  const onSubmit = (data) => {
<% editableColumns.filter(column => column.type === 'Int').forEach(column => { %>
    data.<%= column.name %> = parseInt(data.<%= column.name %>, 10)
<% }) %>
    props.onSave(data, props?.${singularCamelName}?.id)
  }

Generated code without the change above:

Generated code with the change above (plus some whitespace tweaks):

cannikin commented 4 years ago

Hmmm, we have code that handles this in the scaffold, I'm not sure why it wasn't added for you...

https://github.com/redwoodjs/redwood/blob/master/packages/cli/src/commands/generate/scaffold/templates/components/NewName.js.template#L21

So you end up with:

const onSave = (input) => {
  const castInput = Object.assign(input, { num: parseInt(input.num) })
  createPost({ variables: { input: castInput } })
}

What version of Redwood generated that scaffold? I want to say this casting was released in 0.6.0? Maybe 0.7.0?

Tobbe commented 4 years ago

I'm on Redwood 0.7.0

This is the line you linked to const castInput = Object.assign(input, { <% intForeignKeys.forEach(key => { %>${key}: parseInt(input.${key}), <% }) %>})<% } %> It uses intForeignKeys that come from a function in helpers.js

// Returns only relations that are of datatype Int
export const intForeignKeysForModel = (model) => {
  return model.fields
    .filter((f) => f.name.match(/Id$/) && f.type === 'Int')
    .map((f) => f.name)
}

My field name does not end with Id, so it's not included in that array.

cannikin commented 4 years ago

Ahhh that's right, this was only so that foreign key references to other tables would be handled correctly. Well shoot.

We may want to coerse more than ints in the future...what you do think about a higher level check that avoids having to do it inline in the onSubmit handler? We've been looking at integrating Yup which would allow you to define the datatypes of all the fields in your form and then the data object returned to onSubmit would have the proper datatypes automatically: https://react-hook-form.com/advanced-usage/#SchemaValidation

I'm not thrilled about yet another place you have to define your schema datatypes (schema.prisma and GraphQL SDL are already two places), but this Int issue has come up several times and we've got another issue about dealing with Booleans. Adding Yup wouldn't add any overhead on scaffolds since we can generate that schema for you. We'll just have to let people know to either write a coercement manually in onSubmit, as we've been doing above, or write themselves a Yup schema.

Maybe we can even add a little CLI tool that turns the model definition in your schema.prisma into the equivalent Yup schema and outputs it to the terminal. Then you can copy/paste into your component...

@thedavidprice your Yup dreams may be coming true!

Tobbe commented 4 years ago

We may want to coerse more than ints in the future

Yes, this was the first thing that came to my mind as well. I guess there are two ways to think about this. I have a colleague who is firmly in the "if you can't do it properly, don't do it at all"-camp. Myself I'm more in the "anything is better than nothing"-camp.

We've been looking at integrating Yup

I've never used react-hook-form before (we have our own form implementation that we're using), and I've never even heard of yup before. So can't comment much on all that

I'm not thrilled about yet another place you have to define your schema datatypes

I'm 100% with you on that one!

We'll just have to let people know to either write a coercement manually in onSubmit, as we've been doing above, or write themselves a Yup schema.

Or (try to) generate the coercement with the template...

If/when Redwood moves to TypeScript, how relevant would Yup be? I mean, the data object sent to onSubmit would have to be typed already, so maybe that changes things a bit? I guess you'd have to generate the TS interface/type from the schema file. Again, don't know much about react-hook-form, but it looks like the template would have to include a call to r-h-f's useForm, with the TS type information, and then pass that in to <Form formMethods={formMethods} > Does that mean that the data object that we get in onSubmit would already have the correct types for everything? Thus making this a non-issue?

cannikin commented 4 years ago

Does that mean that the data object that we get in onSubmit would already have the correct types for everything? Thus making this a non-issue?

Maybe? I honestly have next to no knowledge of Typescript and how/where/when types are introduced in this chain. I think the idea of Yup is that it would properly enforce and coerce types, so data would be typed properly, but then where does the type definition for TS come from? Do we then need yet another type definition somewhere? AHHHHH

Tobbe commented 4 years ago

I'm in bed now, I'll write up some code tomorrow to show how I think it would/could look with TS support. But yes, another type definition would be needed. Hopefully it can be generated from the Prisma schema though

On Thu, May 28, 2020, 01:08 Rob Cameron notifications@github.com wrote:

Does that mean that the data object that we get in onSubmit would already have the correct types for everything? Thus making this a non-issue?

Maybe? I honestly have next to no knowledge of Typescript and how/where/when types are introduced in this chain. I think the idea of Yup is that it would properly enforce and coerce types, so data would be typed properly, but then where does the type definition for TS come from? Do we then need yet another type definition somewhere? AHHHHH

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/redwoodjs/redwood/issues/608#issuecomment-634990724, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAHQSMJTKFIONSP3UT5OZ3RTWMPTANCNFSM4NLLOMGA .

Tobbe commented 4 years ago

With this schema

model Post {
  id        Int      @id @default(autoincrement())
  title     String
  body      String
  num       Int
  createdAt DateTime @default(now())
}

Parts of the generated form would probably look like this

interface FormData {
  title: string;
  body: string;
  num: number;
}

const PostForm = (props: Props) => {
  const formMethods = useForm<FormData>();

  const onSubmit = (data: FormData) => {
    props.onSave(data, props?.post?.id)
  }

  return (
    <div className="box-border text-sm -mt-4">
      <Form onSubmit={onSubmit} error={props.error} formMethods={formMethods}>
        <FormError
          error={props.error}

The same interface would also be used in NewPost

import PostForm, { FormData } from 'src/components/PostForm'

// ...

  const onSave = (input: FormData) => {
    createPost({ variables: { input } })
  }

So, yeah, I don't think the coercion will be needed in any Redwood code (it'll be handled by react-hook-form).

But that's only for TS code. If you guys want to support generating both JS and TS projects, then something will still be needed for JS projects.

cannikin commented 4 years ago

How does react-hook-form know to do the coercion? Does that happen automatically once you start using Typescript somehow? When we were looking into adding coercion their recommendation was to use Yup...

Tobbe commented 4 years ago

Looking at react-hook-form, it's clear that they don't do any coercion 😞 They say "it's a feature, not a bug" 😛 Look here for example: https://github.com/react-hook-form/react-hook-form/issues/1116

https://codesandbox.io/s/react-hook-form-typescript-d68q4 Fill in the form, and enter an age, 48 for example, and you'd think/hope it would log 58, but it actually logs 4810

So, back at square one? Even with TS we'd have to do our own coercion.

thedavidprice commented 4 years ago

Late to the party but excited to see this conversation in progress! Thanks for jumping in @Tobbe 🚀

I'm also lacking proper TS skills, so a basic question/clarification:

type casting in TS is called [type assertion](const PostForm = (props: Props) => { const formMethods = useForm()); which is done at compile-time, not run-time
in the example code above, this is happening here (specifically the <FormData>):
```
const PostForm = (props: Props) => {
const formMethods = useForm<FormData>();
```

But, in this case, won't we still need to perform the cast/coercion at run-time for both TS and JS given that <input type=number .../> will still be typeOfwill == string?

On it's own, React Hook Form does not do this for us (unless I'm misunderstanding). They give some examples on this page, but it requires Yup.

cannikin commented 4 years ago

It seems very anti-Redwood to have the type definitions for your schema defined in four different places:

schema.prisma
GraphQL SDL
Yup schema for form validation
Typescript schema

We've got another issue that suggests adding a fifth—validations in the service!

This seems like the opposite of everything we're trying to do—make your development experience easier. Any ideas @mojombo?

Tobbe commented 4 years ago

@thedavidprice You are absolutely right. The compiler says the type is number for my codesandbox example above, but runtime it's actually string.

thedavidprice commented 4 years ago

@Tobbe our messages are in sync and just crossed paths! Sorry mine's basically redundant.

I do think this is of high value for Redwood. But not sure how high priority at this time. Regardless, two ideas for ways forward:

roll our own casting/coercion --> we have the fields and types, is it possible (and manageable) to add coercion
try out Yup. The example on the RHF page are specific to Schema Validation and Custom Hook with Validation Resolver, but has it's own cast() method.

Here was a quick search result article on RHF that mentions some pros/cons of also using Yup.

Any interest in trying out a simple spike or even POC?

thedavidprice commented 4 years ago

@cannikin 100% agree. Couple thoughts/mentions:

this proposal from @mohsen1 (with comments from @aldonline) would handle some of the redundancy: https://github.com/redwoodjs/redwood/issues/418
and, given the Yup API, I was imagining we could handle const schema = yup.object().shape(...) behind the scenes automatically. I guess I should have made that idea/curiosity explicit.

Tobbe commented 4 years ago

@cannikin Everyone's first reaction to TS is "OMG, all the extra code, ain't nobody got time for that (╯°□°）╯︵ ┻━┻"

For personal projects I still go for JS, but for anything serious and/or with multiple people I actually think TS is worth it. The time lost to the extra typing (in both senses of the word 😉) is saved thanks to the extra editor help you get, and the fewer bugs introduced.

@thedavidprice spike/POC of our own casting, or of yup? Or both?

thedavidprice commented 4 years ago

Everyone's first reaction to TS is "OMG, all the extra code, ain't nobody got time for that (╯°□°）╯︵ ┻━┻"

^^ This would be representative of my current state of TS "evolution" 😆But been encouraged as I've learned more about how simple employing it can be. And, also, having a hybrid-language project is 100% supported. So baby steps...

spike/POC of our own casting, or of yup? Or both?

🤩^^ If you're up for it, pick your poison! (Or suggest a different path.) But in that case, please don't get too far out in front of @cannikin and me as you go. We'll need to take things in pieces, e.g. just try it on one field/type, and also need to figure out what kind of DX would be appropriate for Redwood (see Rob's comment above). So maybe something like this to start:

put together a simple plan
just focus on Javascript
just focus on a field/type (or small set)
etc.

@cannikin Am I already getting too far ahead here... thoughts/suggestions about how to set this up for (digestible) success?

cannikin commented 4 years ago

HA I come from almost 15 years of Ruby and can probably count on one hand all the type errors I've ever seen because I passed an Integer instead of a String. I don't think anyone can convince me that the additional days/months I would have spent writing type definitions over that time would have been worth it! 😄

What does the editor support look like? I tend to ignore any little box that pops up because any time I've glanced at it it's been some obscure object definition syntax that does me no good for what I'm working on. Or is it a red squiggle? Which I also usually ignore and just deal with the console error when it pops up!

I think we're going to discuss this one in our team meeting on Tuesday and see what we can do. I'll check back in after that!

Tobbe commented 4 years ago

Old dogs, new tricks, and all that... I was the biggest opponent of TS, and nothing anyone could have told me would have convinced me otherwise. Not until I actually tried it for a couple of projects did I actually start appreciating it. But I think we've slipped pretty far away from what this issue was supposed to be about :D

So, to try to bring it back to point, I could start by creating a PR for our own casting of perhaps only numbers and booleans. Ideally someone would also create a PR for a yup solution. So the two can be compared. But I don't want to over commit, so I'll just do one for our own casting first.

cannikin commented 4 years ago

Sounds good! @thedavidprice and I were talking separately and he reminded me of this issue #418 where folks are talking about code generation to make "GraphQL operations type-safe". The ultimate solution to this might be something similar if we want to keep the types in sync with the database schema, but without making the user keep them in sync themselves.

cannikin commented 4 years ago

@Tobbe OKAY we discussed in our weekly meeting and we have a plan!

Rather than go full Yup we think we came up with a lower touch version that doesn't involve any special work on the part of the user, other than making sure they use the correct input Field type—we'll do the coercion for them.

This involves three changes:

In the scaffold generator we create a <NumberField> for any fields where datatype is Int
In web/src/form/form.js where we define the Form component, instead of directly passing onSubmit to handleSubmit, we'd wrap the onSubmit in our own function that does the coercion first. We can loop through props.children to get all the input components and depending on the type (NumberField, CheckboxField, TextField) we know what type to coerce it to.
We add the ability to override the coercion type by adding a type key to the validation object: <TextField validation={{ required: true, type: 'Int' }}>. We look for a type in the validation object and if it exists, use it, otherwise fallback to the type of the input field itself and use that instead.

We can see how far that gets us and if we even need to go to Yup. And as for Typescript, as long as the form data is being used in a mutation @peterp thinks we can automatically use the mutation type as the TS type for the data object. I guess if you're doing something custom with the data you'll need to write the type definition yourself.

There are two PRs that still need to merge, we should probably wait for that before making any changes: https://github.com/redwoodjs/redwood/pull/590 and https://github.com/redwoodjs/redwood/pull/495

Are you still interested in working on this or should I take a stab at it?

Tobbe commented 4 years ago

Sounds like a great plan. I'm very interested in working on it 🙂 Would it be alright if I focused on 1. and 2. first, and then go for 3. in a separate PR? I haven't looked at the validation code yet, and would feel more confident doing the other two changes first.

cannikin commented 4 years ago

Absolutely, thanks again for taking this on!

This PR is nice because it adds a lookup for mapping a field type to the input type that's output in the generator, so we can leverage that for Int: https://github.com/redwoodjs/redwood/pull/495

Then this PR creates a single generic <InputField> that all the other fields are built off of. Now that I think about it this may not actually matter since we should only need to modify <Form> itself and iterate on the children to get the name of the component. That one can probably be ignored!

Tobbe commented 4 years ago

Now that the PR(s) this one depended on have been merged, I'll take a look at this again. Most likely today or tomorrow

Tobbe commented 4 years ago

I've got a branch up with a fix for this. I have not created a PR yet though, as the code depends on #732 Feel free to take a look at the code in my repo in the meantime if you want 🔍

Tobbe commented 4 years ago

I refactored the code a bit, new commit here https://github.com/Tobbe/redwood/commit/e962bf4dfb29b4cf81a4201e630b203e65414274

cannikin commented 4 years ago

@Tobbe just merged https://github.com/redwoodjs/redwood/pull/732

Tobbe commented 4 years ago

@cannikin Thanks for the merge. I've created a proper PR from my branch now. Please take a look. https://github.com/redwoodjs/redwood/pull/749

redwoodjs / redwood

Error when submitting scaffolded form with Int field #608