Skip to content
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

Make Form generic for better TS support #5867

Merged
merged 9 commits into from
Jul 7, 2022

Conversation

Tobbe
Copy link
Member

@Tobbe Tobbe commented Jul 2, 2022

This PR adds generics to <Form> that we pass on to react-hook-form for better/stronger types, especially in the onSubmit callback.

It will allow you to do this:

<Form<FormData>
  onSubmit={(data) => {
    // `data` is correctly typed here, and VSCode IntelliSense will show 
    // available attributes when you type `data.`
    console.log('submitted name:', data.name)
  }}
>
  <TextField name="name" />
  <Submit>Save</Submit>
</Form>

If you pass custom formMethods into the form you should put the type on the hook instead

const formMethods = useForm<FormData>()

return (
  <Form formMethods={formMethods}>
    // ...
  </Form>
)

@netlify
Copy link

netlify bot commented Jul 2, 2022

Deploy Preview for redwoodjs-docs canceled.

Name Link
🔨 Latest commit 98e06dc
🔍 Latest deploy log https://app.netlify.com/sites/redwoodjs-docs/deploys/62c6f34d09a9dc0008c3dd80

@Tobbe Tobbe added the release:feature This PR introduces a new feature label Jul 2, 2022
@Tobbe Tobbe mentioned this pull request Jul 2, 2022
17 tasks
@Tobbe Tobbe requested a review from jtoar July 3, 2022 09:26
Copy link
Contributor

@jtoar jtoar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a double take when I saw <Form<FormData>, but it's a thing: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-9.html#generic-type-arguments-in-jsx-elements.

I confirmed that it works as advertised! I understand your concern about type casting, but I don't understand quite enough about forwardRef to comment on the tradeoff. (I did read the issue linked in the comments, but still.) I think I did try to implement this before back when I typed this file, but didn't know how to pass a generic to a JSX element. Now I do, so LGTM!

packages/forms/src/index.tsx Outdated Show resolved Hide resolved
@Tobbe Tobbe enabled auto-merge (squash) July 7, 2022 14:53
@Tobbe Tobbe merged commit 9db899e into redwoodjs:main Jul 7, 2022
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Jul 7, 2022
@Tobbe Tobbe deleted the tobbe-form-ts-generics branch July 7, 2022 16:13
dac09 added a commit to dac09/redwood that referenced this pull request Jul 12, 2022
…ctmode-gen

* 'main' of github.com:redwoodjs/redwood: (22 commits)
  Remove "node:os" import from telemetry (redwoodjs#5914)
  fix: temporarily disable telemetry on windows (redwoodjs#5899)
  fix(deps): update dependency dotenv-webpack to v7.1.1 (redwoodjs#5909)
  fix(deps): update dependency dotenv-defaults to v5.0.2 (redwoodjs#5908)
  fix(deps): update dependency webpack-dev-server to v4.9.3 (redwoodjs#5906)
  fix(deps): update dependency core-js to v3.23.4 (redwoodjs#5905)
  Fix single-character typo (redwoodjs#5904)
  chore(deps): update dependency esbuild to v0.14.49 (redwoodjs#5898)
  Gitpod hot reload fix (redwoodjs#5888)
  fix(deps): update dependency @types/aws-lambda to v8.10.101 (redwoodjs#5894)
  fix(deps): update dependency @testing-library/user-event to v14.2.1 (redwoodjs#5893)
  fix(deps): update dependency @testing-library/react-hooks to v8.0.1 (redwoodjs#5892)
  chore(deps): update dependency typescript to v4.7.4 (redwoodjs#5886)
  feat: add noire-munich to core team (redwoodjs#5891)
  Fix file structure display (redwoodjs#5889)
  Make Form generic for better TS support (redwoodjs#5867)
  chore(deps): update dependency npm-packlist to v5.1.1 (redwoodjs#5885)
  Bump framer motion version to 6.* as recommended (redwoodjs#5870)
  chore(deps): update dependency nodemon to v2.0.19 (redwoodjs#5884)
  chore(deps): update dependency firebase to v9.8.4 (redwoodjs#5879)
  ...
@jtoar jtoar modified the milestones: next-release, v2.2.0 Jul 22, 2022
@Philzen
Copy link
Contributor

Philzen commented Jul 29, 2022

This looks like awesome stuff @Tobbe (its implementations frankly completely over the top of my head, but as a "user" i always enjoy seeing some type magic happening as soon as i pass the right generic 😃 ).

I'm wondering whether you could put this into the docs so the existence / application of the FormData type is made more aware for people to discover and enjoy it, i.e. in the tutorial or on the forms page

@Tobbe
Copy link
Member Author

Tobbe commented Aug 4, 2022

@Philzen I agree! Wrote up an issue for it #6120

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature This PR introduces a new feature
Projects
Status: Archived
Development

Successfully merging this pull request may close these issues.

3 participants