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

useMutationForm hook #2301

Merged
merged 5 commits into from
Sep 30, 2023
Merged

useMutationForm hook #2301

merged 5 commits into from
Sep 30, 2023

Conversation

berekuk
Copy link
Collaborator

@berekuk berekuk commented Sep 29, 2023

My TypeScript types are getting a bit crazy, tbh.

Submitting it separately from functional changes, so that we can discuss whether I should stop adding more complexity :) (but this is almost the last abstraction I had in mind, I promise).

Before this, we had a few forms which did useForm + useAsyncMutation (which itself is our abstraction on top of Relay's useMutation) + form.handleSubmit(...) to tie them together.

This PR abstracts this pattern under one hook with many parameters.

We already had something like this for dropdown actions since I added <MutationModalAction> in #2260, but it wasn't general enough to use in other forms, like new model and new group pages.

@berekuk berekuk requested a review from OAGr as a code owner September 29, 2023 22:04
@changeset-bot
Copy link

changeset-bot bot commented Sep 29, 2023

⚠️ No Changeset found

Latest commit: b8f2baf

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@berekuk berekuk temporarily deployed to Preview September 29, 2023 22:04 — with GitHub Actions Inactive
@vercel
Copy link

vercel bot commented Sep 29, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
quri-hub ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 30, 2023 3:45am
quri-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 30, 2023 3:45am
squiggle-components ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 30, 2023 3:45am
squiggle-website ✅ Ready (Inspect) Visit Preview Sep 30, 2023 3:45am

defaultValues: initialFormValues,
mutation: graphql`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm still not sure if inlining mutations is good or bad, but started to lean towards it being good. Mutation name (EditSquiggleSnippetModelMutation) and its expectedTypename (UpdateSquiggleSnippetResult) need to be mentioned in generics and other params.

So if the mutation was declared separately outside of the component, you'd have to jump back and forth to change it.

const { form, onSubmit, inFlight } = useMutationForm<
FormShape,
EditSquiggleSnippetModelMutation,
"UpdateSquiggleSnippetResult"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This duplicates the expectedTypename field, and unfortunately it's necessary for type safety. (It's also necessary in useAsyncMutation that we had before, but this hook's type parameters are stricter).

This is a known TypeScript limitation: it lacks partial type argument inference (see microsoft/TypeScript#26242 for the details).

I hope they'll get around to adding it in the next 6-12 months (people have been complaining about its priority for a long time, e.g. microsoft/TypeScript#55486 (comment)), but until then we'll have to tolerate it.

// For `useMutationForm`, it'd be useful to pass it to `onCompleted` explicitly, `onCompleted(result, submittedData)`.
Pick<
Parameters<ReturnType<typeof useAsyncMutation<TMutation, TTypename>>[0]>[0],
"onCompleted"
Copy link
Collaborator Author

@berekuk berekuk Sep 29, 2023

Choose a reason for hiding this comment

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

This is the most complex type in this PR, so I'll explain it as an example of what's going on and why.

useAsyncMutation returns runMutation function, which you can call to actually run the mutation. With this hook, it runs on form submit.

runMutation takes variables parameter (almost always), but also onCompleted callback to run if the mutation succeeded.

In this hook, though, since runMutation is invoked when the form is submitted, onCompleted callback needs to be passed in immediately when "mutation form" is created.

onCompleted type is complicated: its parameter is OkResult, defined inside useAsyncMutation. It's inferred from the mutation result type (generated by Relay), but filtered for expectedTypename.

I could write out the full onCompleted function type here, and it would work, but that would be a copy-paste from useAsyncMutation. So instead, I infer the type from useAsyncMutation type itself.

  • typeof useAsyncMutation<TMutation, TTypename> — this limits useAsyncMutation to its specialized non-generic version
  • ReturnType<...>[0] extracts runMutation type
  • Parameters<...>[0] extracts runMutation's first parameter
  • Pick<..., "onCompleted"> extract { onCompleted } type

Copy link
Contributor

@OAGr OAGr left a comment

Choose a reason for hiding this comment

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

Looks a bit gnarly, but forms can be like that. It's good though that most of the complexity is at least contained to useMutationForm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants