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

useApiMutation hook #48

Merged
merged 10 commits into from
Apr 13, 2020
Merged

useApiMutation hook #48

merged 10 commits into from
Apr 13, 2020

Conversation

thomasdashney
Copy link
Contributor

@thomasdashney thomasdashney commented Apr 11, 2020

Added useApiMutation hook (resolves #28):

const [createUser, {mutating: creatingUser}] = useApiMutation({
  mutation: (firstName: string, lastName: string) => async (api) => {
    return api.request(UserEndpoints.create({firstName, lastName}))
  },
  onError: (error) => console.error(error),
  onSuccess: (user) => console.log(`Created user ${firstName}`)
})

// usage
createUser('Thomas', 'Dashney')

Notes about API decisions:

  • Mutation arguments can be whatever you want. The mutation property is curried so it can pass api helpers (a subset of api class methods), primarily so that the types can be inferred for the mutation function (createUser in the example)
  • onError reduces the need to add a try/catch block
  • onSuccess allows for logic to run once the mutation completes. It's worth noting that if an error occurs inside the onSuccess handler, onError will not trigger. This is important because the error handler should only apply to the actual mutation operation.
  • request's default fetch policy is no-cache by default, but can be overridden via ApiProvider or per-request.

Removed Api#defaultFetchPolicy (resolves #37)

  • Now that we have mutations, I can't think of a good reason to keep defaultFetchPolicy, since this was just added for mutations. Also, this now defaults to no-cache (which I think is the most sensible default for making the rare raw api call).

@codeclimate
Copy link

codeclimate bot commented Apr 11, 2020

Code Climate has analyzed commit 3767d87 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (100% is the threshold).

This pull request will bring the total coverage in the repository to 100.0% (0.0% change).

View more on Code Climate.

deregisterMutation()
}

params.onSuccess?.(returnValue, {mutationArgs})
Copy link

Choose a reason for hiding this comment

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

Is there a reason this needs to run after the try-catch block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This way, if an error occurs inside the onSuccess handler, onError will not trigger. This could avoid situations where a success snackbar shows via onSuccess, but then an error occurs after that, causing onError to be called and immediately triggering an error snackbar. I figure it's more clear that onError is only called if the actual mutation fails

TBH I was originally not going to add the onSuccess callback at all, but I think the above distinction justifies it's usefulness. It's also consistent with other popular data-fetching libraries. LMK what you think

Copy link

Choose a reason for hiding this comment

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

Makes sense!

@azmenak azmenak merged commit 91db4b0 into master Apr 13, 2020
@azmenak azmenak deleted the use-api-mutation branch April 13, 2020 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default api.request#defaultFetchPolicy to no-cache Mutations
2 participants