-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add full types to useMutation's returned callback #1014
Changes from 6 commits
a42105b
36ecc80
2bb34a9
3b56f57
b377eb3
1a44f22
d7e83f0
07a430b
b47b6fc
66dd9bb
dc9fc32
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -68,7 +68,7 @@ type UseQueryResult<T> = { | |||||
isLoading: false; // Query is currently loading for the first time. No data yet. | ||||||
isFetching: false; // Query is currently fetching, but might have data from an earlier request. | ||||||
isSuccess: false; // Query has data from a successful load. | ||||||
isError: false; // Query is currently in "error" state. | ||||||
isError: false; // Query is currently in an "error" state. | ||||||
|
||||||
refetch: () => void; // A function to force refetch the query | ||||||
}; | ||||||
|
@@ -91,18 +91,39 @@ The query arg is used as a cache key. Changing the query arg will tell the hook | |||||
#### Signature | ||||||
|
||||||
```ts | ||||||
type MutationHook = () => [ | ||||||
( | ||||||
arg: any | ||||||
) => { | ||||||
unwrap: () => Promise<ResultTypeFrom<D>>; | ||||||
}, | ||||||
MutationSubState<D> & RequestStatusFlags | ||||||
]; | ||||||
type UseMutation<Definition> = () => [UseMutationTrigger<Definition>, UseMutationResult<Definition>]; | ||||||
|
||||||
type UseMutationTrigger<Definition> = ( | ||||||
arg: ArgTypeFrom<Definition> | ||||||
) => Promise<{ data: ResultTypeFrom<Definition> }> & { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
could be a bit more accurate thatn There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, went with this option: ) => Promise<{ data: ResultTypeFrom<Definition> } | { error: BaseQueryError | SerializedError }> & { |
||||||
arg: { | ||||||
endpointName: string; // The name of the given endpoint for the mutation | ||||||
originalArgs: ArgTypeFrom<Definition>; // Arguments passed to the mutation | ||||||
track?: boolean; // Whether the mutation is being tracked in the store | ||||||
startedTimeStamp: number; // Timestamp for when the mutation was initiated | ||||||
}; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is kinda "implementation detail". Could we mark that as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep sounds good to me |
||||||
requestId: string; // A string generated by RTK Query | ||||||
abort: () => void; // A method to cancel the mutation promise | ||||||
unwrap: () => Promise<ResultTypeFrom<Definition>>; // A method to unwrap the mutation call and provide the raw response/error | ||||||
unsubscribe: () => void; // A method to manually unsubscribe from the mutation call | ||||||
}; | ||||||
|
||||||
type UseMutationResult<Definition> = { | ||||||
data?: ResultTypeFrom<Definition>; // Returned result if present | ||||||
endpointName?: string; // The name of the given endpoint for the mutation | ||||||
error?: any; // Error result if present | ||||||
fulfilledTimestamp?: number; // Timestamp for when the mutation was completed | ||||||
isError: boolean; // Mutation is currently in an "error" state | ||||||
isLoading: boolean; // Mutation has been fired and is awaiting a response | ||||||
isSuccess: boolean; // Mutation has data from a successful call | ||||||
isUninitialized: boolean; // Mutation has not been fired yet | ||||||
originalArgs?: ArgTypeFrom<Definition>; // Arguments passed to the latest mutation call | ||||||
startedTimeStamp?: number; // Timestamp for when the latest mutation was initiated | ||||||
}; | ||||||
``` | ||||||
|
||||||
- **Returns**: a tuple containing: | ||||||
- `trigger`: a function that triggers an update to the data based on the provided argument | ||||||
- `trigger`: a function that triggers an update to the data based on the provided argument. The trigger function returns a promise with the properties shown above that may be used to handle the behaviour of the promise. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your non-American behavior here is silly! 🤣 |
||||||
- `mutationState`: a query status object containing the current loading state and metadata about the request | ||||||
|
||||||
#### Description | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,12 +11,13 @@ import { | |
AsyncThunk, | ||
ThunkAction, | ||
unwrapResult, | ||
SerializedError, | ||
} from '@reduxjs/toolkit' | ||
import { QuerySubState, SubscriptionOptions } from './apiState' | ||
import { InternalSerializeQueryArgs } from '../defaultSerializeQueryArgs' | ||
import { Api } from '../apiTypes' | ||
import { ApiEndpointQuery } from './module' | ||
import { BaseQueryResult } from '../baseQueryTypes' | ||
import { BaseQueryError } from '../baseQueryTypes' | ||
|
||
declare module './module' { | ||
export interface ApiEndpointQuery< | ||
|
@@ -79,18 +80,98 @@ type StartMutationActionCreator< | |
export type MutationActionCreatorResult< | ||
D extends MutationDefinition<any, any, any, any> | ||
> = Promise< | ||
ReturnType< | ||
BaseQueryResult< | ||
D extends MutationDefinition<any, infer BaseQuery, any, any> | ||
? BaseQuery | ||
: never | ||
> | ||
> | ||
| { data: ResultTypeFrom<D> } | ||
| { | ||
error: | ||
| BaseQueryError< | ||
D extends MutationDefinition<any, infer BaseQuery, any, any> | ||
? BaseQuery | ||
: never | ||
> | ||
| SerializedError | ||
} | ||
> & { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @phryneas I've tried fixing the type for the resolved value of the 'trigger' promise here, but the actual Can you make any sense out of that? I've temporarily suppressed the error in order to allow codesandbox to build, in order to see how it behaved there: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah it's just occurred to me; would that be caused by the scenario where the | BaseQueryError<
D extends MutationDefinition<any, infer BaseQuery, any, any>
? BaseQuery
: never
> There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm particularly stumped because it looks like export type BaseQueryError<BaseQuery extends BaseQueryFn> = Exclude<
UnwrapPromise<ReturnType<BaseQuery>>,
{ error: undefined }
>['error'] There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I pushed a new version which explicitly removes Promise<
| { data: ResultTypeFrom<D> }
| {
error:
| Exclude<
BaseQueryError<
D extends MutationDefinition<any, infer BaseQuery, any, any>
? BaseQuery
: never
>,
undefined
>
| SerializedError
}
> But I'm iffy about two things:
|
||
arg: QueryArgFrom<D> | ||
/** @internal */ | ||
arg: { | ||
Shrugsy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/** | ||
* The name of the given endpoint for the mutation | ||
*/ | ||
endpointName: string | ||
/** | ||
* The original arguments supplied to the mutation call | ||
*/ | ||
originalArgs: QueryArgFrom<D> | ||
/** | ||
* Whether the mutation is being tracked in the store. | ||
*/ | ||
track?: boolean | ||
/** | ||
* Timestamp for when the mutation was initiated | ||
*/ | ||
startedTimeStamp: number | ||
} | ||
phryneas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/** | ||
* A unique string generated for the request sequence | ||
*/ | ||
requestId: string | ||
/** | ||
* A method to cancel the mutation promise. Note that this is not intended to prevent the mutation | ||
* that was fired off from reaching the server, but only to assist in handling the response. | ||
* | ||
* Calling `abort()` prior to the promise resolving will make it throw with an error like: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually have no idea what that would do, have you tested that? I assume it would resolve with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yeah my wording is definitely off. When I wrote it, in my head I was specifically thinking of the unwrapped promise, but that's not what I actually wrote. I'll re-word it to make sense There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Re-worded to refer to 'error state' rather than saying it will 'throw' There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also fwiw, it behaves exactly like you thought for the 'wrapped' promise |
||
* `{ name: 'AbortError', message: 'Aborted' }` | ||
* | ||
* @example | ||
* ```ts | ||
* const [updateUser] = useUpdateUserMutation(); | ||
* | ||
* useEffect(() => { | ||
* const promise = updateUser(id); | ||
* promise | ||
* .unwrap() | ||
* .catch((err) => { | ||
* if (err.name === 'AbortError') return; | ||
* // else handle the unexpected error | ||
* }) | ||
* | ||
* return () => { | ||
* promise.abort(); | ||
* } | ||
* }, [id, updateUser]) | ||
* ``` | ||
*/ | ||
abort(): void | ||
/** | ||
* Unwraps a mutation call to provide the raw response/error. | ||
* | ||
* @remarks | ||
* If you need to access the error or success payload immediately after a mutation, you can chain .unwrap(). | ||
* | ||
* @example | ||
* ```ts | ||
* // codeblock-meta title="Using .unwrap" | ||
* addPost({ id: 1, name: 'Example' }) | ||
* .unwrap() | ||
* .then((payload) => console.log('fulfilled', payload)) | ||
* .catch((error) => console.error('rejected', error)); | ||
* ``` | ||
* | ||
* @example | ||
* ```ts | ||
* // codeblock-meta title="Using .unwrap with async await" | ||
* try { | ||
* const payload = await addPost({ id: 1, name: 'Example' }).unwrap(); | ||
* console.log('fulfilled', payload) | ||
* } catch (error) { | ||
* console.error('rejected', error); | ||
* } | ||
* ``` | ||
*/ | ||
unwrap(): Promise<ResultTypeFrom<D>> | ||
/** | ||
* A method to manually unsubscribe from the mutation call, meaning it will be removed from cache after the usual caching grace period. | ||
The value returned by the hook will reset to `isUninitialized` afterwards. | ||
*/ | ||
unsubscribe(): void | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ import { rest } from 'msw' | |
import { | ||
actionsReducer, | ||
expectExactType, | ||
expectType, | ||
matchSequence, | ||
setupApiStore, | ||
useRenderCounter, | ||
|
@@ -14,6 +15,7 @@ import { | |
import { server } from './mocks/server' | ||
import { AnyAction } from 'redux' | ||
import { SubscriptionOptions } from '../core/apiState' | ||
import { SerializedError } from '../../createAsyncThunk' | ||
|
||
// Just setup a temporary in-memory counter for tests that `getIncrementedAmount`. | ||
// This can be used to test how many renders happen due to data changes or | ||
|
@@ -26,6 +28,16 @@ const api = createApi({ | |
if (arg?.body && 'amount' in arg.body) { | ||
amount += 1 | ||
} | ||
|
||
if (arg?.body && 'forceError' in arg.body) { | ||
return { | ||
error: { | ||
status: 500, | ||
data: null, | ||
}, | ||
} | ||
} | ||
|
||
return { | ||
data: arg?.body | ||
? { ...arg.body, ...(amount ? { amount } : {}) } | ||
|
@@ -44,7 +56,7 @@ const api = createApi({ | |
}, | ||
}), | ||
}), | ||
updateUser: build.mutation<any, { name: string }>({ | ||
updateUser: build.mutation<{ name: string }, { name: string }>({ | ||
query: (update) => ({ body: update }), | ||
}), | ||
getError: build.query({ | ||
|
@@ -804,6 +816,84 @@ describe('hooks tests', () => { | |
) | ||
) | ||
}) | ||
|
||
test('useMutation hook callback returns various properties to handle the result', async () => { | ||
function User() { | ||
const [updateUser] = api.endpoints.updateUser.useMutation() | ||
const [successMsg, setSuccessMsg] = React.useState('') | ||
const [errMsg, setErrMsg] = React.useState('') | ||
const [isAborted, setIsAborted] = React.useState(false) | ||
|
||
const handleClick = async () => { | ||
const res = updateUser({ name: 'Banana' }) | ||
|
||
// no-op simply for clearer type assertions | ||
res.then((result) => { | ||
// currently passing with a false positive, because error is getting typed as `any` | ||
expectExactType< | ||
| { | ||
error: { status: number; data: unknown } | SerializedError | ||
} | ||
| { | ||
data: { | ||
name: string | ||
} | ||
} | ||
// @ts-expect-error TEMPORARY ONLY - suppressed for codesandbox build | ||
>(result) | ||
}) | ||
|
||
expectType<{ | ||
endpointName: string | ||
originalArgs: { name: string } | ||
track?: boolean | ||
startedTimeStamp: number | ||
}>(res.arg) | ||
expectType<string>(res.requestId) | ||
expectType<() => void>(res.abort) | ||
expectType<() => Promise<{ name: string }>>(res.unwrap) | ||
expectType<() => void>(res.unsubscribe) | ||
|
||
// abort the mutation immediately to force an error | ||
res.abort() | ||
res | ||
.unwrap() | ||
.then((result) => { | ||
expectType<{ name: string }>(result) | ||
setSuccessMsg(`Successfully updated user ${result.name}`) | ||
}) | ||
.catch((err) => { | ||
setErrMsg( | ||
`An error has occurred updating user ${res.arg.originalArgs.name}` | ||
) | ||
if (err.name === 'AbortError') { | ||
setIsAborted(true) | ||
} | ||
}) | ||
} | ||
|
||
return ( | ||
<div> | ||
<button onClick={handleClick}>Update User and abort</button> | ||
<div>{successMsg}</div> | ||
<div>{errMsg}</div> | ||
<div>{isAborted ? 'Request was aborted' : ''}</div> | ||
</div> | ||
) | ||
} | ||
|
||
render(<User />, { wrapper: storeRef.wrapper }) | ||
expect(screen.queryByText(/An error has occurred/i)).toBeNull() | ||
expect(screen.queryByText(/Successfully updated user/i)).toBeNull() | ||
expect(screen.queryByText('Request was aborted')).toBeNull() | ||
|
||
fireEvent.click( | ||
screen.getByRole('button', { name: 'Update User and abort' }) | ||
) | ||
await screen.findByText('An error has occurred updating user Banana') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Appreciate you keeping the 🍌 s around 💯 |
||
expect(screen.queryByText(/Successfully updated user/i)).toBeNull() | ||
screen.getByText('Request was aborted') | ||
}) | ||
}) | ||
|
||
describe('usePrefetch', () => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -247,7 +247,7 @@ describe('queryFn base implementation tests', () => { | |
{ | ||
const thunk = mutationWithNeither.initiate('mutationWithNeither') | ||
const result = await store.dispatch(thunk) | ||
expect(result.error).toEqual( | ||
expect((result as any).error).toEqual( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI this change is because { data: string; } | { error: string | SerializedError } If anyone has a better idea for handling this than There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Decided on a better option than expect('error' in result && result.error).toEqual( |
||
expect.objectContaining({ | ||
message: 'endpointDefinition.queryFn is not a function', | ||
}) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know the best way to write this, but like this it is omitting
selectFromResult
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added in a pseudo-type for
UseMutationStateOptions
, but it's definitely getting very crowded. What are your thoughts on removing theDefinition
pseudo-generic from this section completely? My thoughts are that it's already an inaccurate generic compared to the real types, and it probably clutters the explanation more than it clarifies anything.