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

Add full types to useMutation's returned callback #1014

Conversation

Shrugsy
Copy link
Collaborator

@Shrugsy Shrugsy commented Apr 24, 2021

useMutation returns a 'trigger function' as it's first item in the return tuple.
This PR changes the signature of the value returned by that 'trigger function' to include all properties it actually has, including that it returns a Promise, not a plain object.

e.g.

Before - only unwrap

image

After - all properties

image

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 24, 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 dc9fc32:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration
rsk-github-issues-example Configuration

},
MutationSubState<D> & RequestStatusFlags
];
type UseMutation<Definition> = () => [UseMutationTrigger<Definition>, UseMutationResult<Definition>];
Copy link
Member

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

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've added in a pseudo-type for UseMutationStateOptions, but it's definitely getting very crowded. What are your thoughts on removing the Definition 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.

query-old/docs/api/created-api/hooks.md Outdated Show resolved Hide resolved

type UseMutationTrigger<Definition> = (
arg: ArgTypeFrom<Definition>
) => Promise<{ data: ResultTypeFrom<Definition> }> & {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
) => Promise<{ data: ResultTypeFrom<Definition> }> & {
) => Promise<{ data: ResultTypeFrom<Definition> | { error: unknown } }> & {

could be a bit more accurate thatn unknown, but definitely mention the error here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, went with this option:

) => Promise<{ data: ResultTypeFrom<Definition> } | { error: BaseQueryError | SerializedError }> & {

Comment on lines 99 to 104
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
};
Copy link
Member

Choose a reason for hiding this comment

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

This is kinda "implementation detail". Could we mark that as @internal in the docblock and omit it here instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep sounds good to me

* 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:
Copy link
Member

Choose a reason for hiding this comment

The 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 { error: { name: 'AbortError', message: 'Aborted' } unless you unwrapped it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also fwiw, it behaves exactly like you thought for the 'wrapped' promise

src/query/core/buildInitiate.ts Show resolved Hide resolved
src/query/core/buildInitiate.ts Show resolved Hide resolved
src/query/core/buildInitiate.ts Outdated Show resolved Hide resolved
@Shrugsy Shrugsy marked this pull request as draft April 24, 2021 10:14
@Shrugsy Shrugsy changed the title Add full types to useMutation's returned callback [DRAFT] - Add full types to useMutation's returned callback Apr 24, 2021
Comment on lines 82 to 93
> = 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
}
> & {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 error type produced has an additional undefined type attached to it.

image

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:
https://codesandbox.io/s/vanilla-typescript-forked-duy39?file=/src/index.ts

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 extends MutationDefinition ternary fails?

        | BaseQueryError<
            D extends MutationDefinition<any, infer BaseQuery, any, any>
              ? BaseQuery
              : never
          >

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 particularly stumped because it looks like BaseQueryError is explicitly intended to remove the case with { error: undefined }

export type BaseQueryError<BaseQuery extends BaseQueryFn> = Exclude<
  UnwrapPromise<ReturnType<BaseQuery>>,
  { error: undefined }
>['error']

Copy link
Collaborator Author

@Shrugsy Shrugsy Apr 24, 2021

Choose a reason for hiding this comment

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

I pushed a new version which explicitly removes undefined from the type returned by BaseQueryError like so:

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:

  • Is undefined actually supposed to be there for some reason?
  • If not, and I remove it doing the above, and someone defines their error type like: { error: undefined }, then the promise type will be wrong (Because it will remove undefined from the union, leaving only SerializedError)

@@ -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(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI this change is because result previously got inferred as any, but is now more accurately:

{ data: string; } | { error: string | SerializedError }

If anyone has a better idea for handling this than any-casting I'm all ears

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Decided on a better option than as any:

      expect('error' in result && result.error).toEqual(

@Shrugsy Shrugsy changed the title [DRAFT] - Add full types to useMutation's returned callback Add full types to useMutation's returned callback Apr 24, 2021
@Shrugsy Shrugsy marked this pull request as ready for review April 24, 2021 12:26
Copy link
Collaborator

@markerikson markerikson left a comment

Choose a reason for hiding this comment

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

LGTM, but would like a 👍 from @phryneas or @msutkowski to confirm


- **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.
Copy link
Member

Choose a reason for hiding this comment

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

Your non-American behavior here is silly! 🤣

fireEvent.click(
screen.getByRole('button', { name: 'Update User and abort' })
)
await screen.findByText('An error has occurred updating user Banana')
Copy link
Member

Choose a reason for hiding this comment

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

Appreciate you keeping the 🍌 s around 💯

@markerikson markerikson merged commit b74a529 into reduxjs:feature/v1.6-integration Apr 24, 2021

The generated `UseMutation` hook will cause a component to re-render by default after the trigger callback is fired as it affects the properties of the result. If you want to call the trigger but don't care about subscribing to the result with the hook, you can use the `selectFromResult` option to limit the properties that the hook cares about.

Passing a completely empty object will prevent the hook from causing a re-render at all, e.g.
Copy link
Member

@phryneas phryneas Apr 24, 2021

Choose a reason for hiding this comment

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

Even giving selectFromResult, starting a mutation will trigger at least one re-render.

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

4 participants