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

feat: Hook for remote mutations #1450

Merged
merged 34 commits into from Apr 11, 2022
Merged

feat: Hook for remote mutations #1450

merged 34 commits into from Apr 11, 2022

Conversation

shuding
Copy link
Member

@shuding shuding commented Sep 12, 2021

Closes #1262.

Motivation

There are a lot of existing discussions and feature requests about doing requests with side effects on the remote side manually in SWR as well as a better mutation API design (e.g. #93, #116, #157, #550, #1082), and we do see some valid use cases for it.

As a conclusion, we need something that:

  • Stops revalidate automatically (mount, focus, reconnect, retry...)
  • Returns a callback function for triggering the mutation manually
  • Prevents potential race conditions
  • Revalidates the resource after mutation
  • Backfills the return value to the cache
  • Deriving: Backfills the cache based on the response, and then triggers a revalidation
  • Expose isMutating for useSWR
  • Supports optimistic UI functionalities (feat: Optimistic mutation with error rollback #1745)

Breakings

useSWRMutation

import useSWRMutation from 'swr/mutation'

async function getData(url, { arg: token }) {
  ... // Fetcher implementation.
      // The extra argument will be passed via the `arg` property of the 2nd parameter.
}

// A useSWR + mutate like API, but it will never start the request.
const { data, error, trigger, reset, isMutating } = useSWRMutation('/api/user', getData, {
  // options
  onError,
  onSuccess,
  revalidate = true, // auto revalidate after mutation
  populateCache = false, // write back the response to the cache after mutation
  optimisticData,
  rollbackOnError
})

For revalidate, populateCache, optimisticData and rollbackOnError, check https://github.com/vercel/swr/releases/tag/1.2.0 and successor releases.

Unlike useSWR, there is no more isValidating and mutate returned from this hook.

trigger

// Trigger it manually, the extra argument will be passed to the mutator as `{ arg }`
try {
  const result = await trigger('my_token')
} catch (error) {
  // ...
}

// override options of `useSWRMutation`
await trigger('my_token', { revalidate: false, populateCache: true })

// in-place event handler
await trigger('my_token', { onError: () => {}, onSuccess: () => {} })

During a mutation, the isMutating state will be true, and any race conditions of useSWR and useSWRMutation will be carefully avoided.

reset

// reset the state (data, error, isMutating)
reset()

To-dos

  • Finalize the name of the hook.
  • Sometimes, mutation APIs (POST, PUT, DELETE) return different payload from the GET API. It might be an issue if we write back to the cache with the same key. This behavior needs to be reconsidered.
  • If the mutation APIs' results cannot be cached, that normally means that we need to revalidate those keys again if there's a useSWR hook with the same key. This can be another option to consider.
  • Ensure callbacks and isMutating state are working as expected.
  • Ensure with a test case that under trigger mode there's no deduplication.
  • Ensure with a test case that trigger prevents race conditions with normal GET requests.
  • Tests for reset.
  • Potential race conditions of reset and trigger calls.
  • populateCache (feat: Add populateCache option to mutate #1729).
  • Improving immediate mutate + later revalidation #157 (feat: Optimistic mutation with error rollback #1745).
  • Polish event callbacks including onDiscarded for this mutation hook.
  • (docs) Have a specific list of options, that can be used in useSWRMutation and ignore the others.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 12, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 76d7864:

Sandbox Source
SWR-Basic Configuration
SWR-States Configuration
SWR-Infinite Configuration
SWR-SSR Configuration

@shuding shuding requested a review from huozhi September 12, 2021 21:46
@shuding shuding marked this pull request as ready for review September 13, 2021 14:32
@promer94
Copy link
Collaborator

How about

const { mutate, trigger, reset, isMutating } = useSWRMutation('/api/user', (extraArgument) => {})
// fetcher only receive extraArgument from trigger
trigger(extraArgument)

trigger would have batter typescript intellisense and for some non-restful endpoints, they actually may not need the key args

mutation/index.ts Outdated Show resolved Hide resolved
@shuding
Copy link
Member Author

shuding commented Sep 15, 2021

fetcher only receive extraArgument from trigger

Yeah that makes the design so much simpler and I have thought about it. But I wasn't sure, and this design is mostly for consistency.

@promer94
Copy link
Collaborator

IMO The benefit of receiving keys is that user could configure a general fetcher in context rather than writing it at every hook instance.

But in useSWRMutation user cannot set a general mutate function in context (or should we allow user to do that ?).

The mutate function could access the keys directly in every hook instance instead of receiving it from its arguments.

@shuding
Copy link
Member Author

shuding commented Sep 15, 2021

I feel that it's still able to reuse the mutator function:

const { trigger } = useSWRMutation('/api/user', post)

trigger({
  name: 'yo'
})

And post is defined as:

function post(API, data) {
  return fetch(API, {
    method: 'POST',
    body: JSON.stringify(data)
  }).then(res => res.json())
}

Which is a general utility to post data.

@promer94
Copy link
Collaborator

I am sorry. my last reply is somehow misleading.

My expectation

// if user already have a mutator function like this
interface User {
  name: string
  age: number
}
const updateUser = (param: User) => post('/api/user', param)

const { trigger } = useSWRMutation('/api/user', updateUser)
// ts error here since age is string
trigger({ name: '123' , age: '12'})

It would be batter if swr could make use of user-defined types and let typescript do type check on trigger.

Possible solution

Since we would not use SWRConfig to set a general mutator function

<SWRConfig 
  value={{
    fetcher: post
   }}>
</SWRConfig>

The key does not have to be passed to mutator function

We could improve the type of trigger by letting mutator function only receive extraArgument. In this way it doesn't need to upgrade the typescript version.

@shuding
Copy link
Member Author

shuding commented Sep 16, 2021

Since we would not use SWRConfig to set a general mutator function

This is a good point! This hook shouldn’t using the fetcher from the context. Will get it fixed. I've reconsidered this, most of time people would pass per-hook mutators, but it also makes sense to provide a global one via the context, in some special cases.

We could improve the type of trigger by letting mutator function only receive extraArgument.

Yeah this can maybe fix the typing problem, but my point was that this breaks the consistency in API design. useSWR passes the key to fetcher and it doesn’t feel great to not do so for the mutator. Does newer TS support this feature?

@promer94
Copy link
Collaborator

Does newer TS support this feature?

It could do the trick. But it would require user to do some extra work

interface User {
  name: string
  age: number
}
const updateUser = (param: User) => post('/api/user', param)

// developer need to type the mutator function explicitly so that trigger could get correct inferred type
const { trigger } = useSWRMutation('/api/user', (key: string, extraArguments: User) => updateUser(extraArguments))
trigger({ name: '123' , age: '12'})

FYI: typescript-playground

@anothertempore
Copy link
Contributor

A question: do you consider supporting configure onSuccess, onError, etc globally? Like what useSWR does.

@shuding
Copy link
Member Author

shuding commented Sep 20, 2021

@anothertempore Yes those should be supported!

src/utils/state.ts Outdated Show resolved Hide resolved
mutation/index.ts Outdated Show resolved Hide resolved
mutation/index.ts Outdated Show resolved Hide resolved
mutation/index.ts Outdated Show resolved Hide resolved
mutation/index.ts Outdated Show resolved Hide resolved
mutation/index.ts Outdated Show resolved Hide resolved
mutation/index.ts Outdated Show resolved Hide resolved
mutation/index.ts Outdated Show resolved Hide resolved
@shuding shuding added this to the 2.0 milestone Mar 2, 2022
src/utils/state.ts Outdated Show resolved Hide resolved
mutation/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@huozhi huozhi left a comment

Choose a reason for hiding this comment

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

Looks great! Amazing work

@shuding shuding merged commit 5aaaf82 into main Apr 11, 2022
@shuding shuding deleted the mutation branch April 11, 2022 21:13
himanshiLt pushed a commit to himanshiLt/swr that referenced this pull request Apr 26, 2022
* (wip) initial impl.

* fix test

* (wip) fix deps

* initial implementation

* fix linter

* fix state reset

* avoid reset race condition

* fix race conditions

* code tweaks

* code tweaks

* return bound mutate

* apply review comments

* fix tsconfig

* type fixes

* fix lint errors

* code tweaks

* fix type error

* update types

* inline serialization result

* merge main and update argument api

* add tests

* fix tests

* update typing

* update state api

* change error condition
@koba04 koba04 mentioned this pull request Aug 5, 2022
13 tasks
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.

None yet

5 participants