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

Breaking Changes in 2.8.0 for TypeScript users? #903

Closed
MarcusNotheis opened this issue Aug 21, 2020 · 9 comments
Closed

Breaking Changes in 2.8.0 for TypeScript users? #903

MarcusNotheis opened this issue Aug 21, 2020 · 9 comments

Comments

@MarcusNotheis
Copy link

Describe the bug
It looks like that there are breaking changes in 2.8.0 for TypeScript users in useMutation:

In 2.7.1, use Mutation was typed as:

export function useMutation<TResult, TVariables = undefined, TError = Error, TSnapshot = unknown>(
  mutationFn: MutationFunction<TResult, TVariables, TError, TSnapshot>,
  mutationOptions?: MutationOptions<TResult, TVariables, TError, TSnapshot>
): MutationResultPair<TResult, TVariables, TError>

https://unpkg.com/browse/react-query@2.7.1/types/index.d.ts

Now in 2.8.0, TVariables and TError have swapped their places:

export declare function useMutation<TResult, TError = unknown, TVariables = undefined, TSnapshot = unknown(
  mutationFn: MutationFunction<TResult, TVariables>, 
  config?: MutationConfig<TResult, TError, TVariables, TSnapshot>
): MutationResultPair<TResult, TError, TVariables, TSnapshot>;

https://unpkg.com/browse/react-query@2.8.0/types/react/useMutation.d.ts

This results in TS Errors for the code which was working in 2.7.1:

return useMutation<void, { items: string[]; otherItems: string[] }>(async ({ items, otherItems }) => {
...
});

Error: TS2339: Property 'items' does not exist on type 'undefined'.

By adding e.g. any as second generic to the useMutation function everything is working as expected again.

Expected behavior
I would have expected that the signature of the method didn't change in a minor release. Anyhow, I think this is not a really big issue which would require a rollback of that change, but it should be mentioned in the release for example so that TypeScript users know how to deal with this issue.

@Daniel-Griffiths
Copy link

I encountered the same issue.

@Garbanas
Copy link
Contributor

Also QueryOptions is now QueryConfig and refetch has lost all its arguments (like throwOnError)...

@boschni
Copy link
Collaborator

boschni commented Aug 21, 2020

Hi @MarcusNotheis! The result and error parameters have indeed been swapped to match with the useQuery signature. But if you type the mutation function then it should not be needed to explicitly define the generics:

useMutation(async ({ items, otherItems }: { items: string[]; otherItems: string[] }) => 'result')

The refetch function has indeed lost its arguments because the function actually did not accept any arguments.

Garbanas added a commit to Garbanas/react-query that referenced this issue Aug 21, 2020
…ch fails

Enables the `throwOnError` configuration option for `useQuery.refetch` that has
as of yet only been present in the documentation but not in the code.

Relates to: TanStack#903 TanStack#843
@bmaclean
Copy link

Changing the useMutation type signature to a union return type with | undefined has propagated through all components that weren't previously handling an undefined return type. I'm assuming it was due to the addition of this line:

20bbedf#diff-eeb1adcf0c71d7ca0cd3b2e1e8fe03f9R181

where the catch block wasn't previously returning. Is this the expected behaviour, where usages of useMutation's mutate function should be handling an undefined return value when the mutation function throws? Thanks!

@boschni
Copy link
Collaborator

boschni commented Aug 21, 2020

Hi @bmaclean! The runtime behaviour has not changed with the new types, meaning the useMutation hook will indeed return undefined by default (see also #810). Unfortunately the previous types were not always correct. The same thing applies to functions like prefetchQuery. Personally I would also expect any returned promises to always resolve or reject by default. Maybe this behaviour should be changed?

tannerlinsley pushed a commit that referenced this issue Aug 23, 2020
#907)

* feat(useQuery): allow useQuery.refetch to throw an error when the fetch fails

Enables the `throwOnError` configuration option for `useQuery.refetch` that has
as of yet only been present in the documentation but not in the code.

Relates to: #903 #843

* refactor(useQuery): move "useQuery.refetch" options to dedicated type
@ThibautMarechal
Copy link

I have created function that use queryCache.prefetchQuery to reuse common code.
I wanted to let the user able to fully configure the prefetchQuery with QueryConfig & PrefetchQueryOptions.
But the interface PrefetchQueryOptions isn't exported anymore.

@bmaclean
Copy link

@ThibautMarechal I believe you're looking for PrefetchQueryObjectConfig

@tannerlinsley
Copy link
Collaborator

Yes, the types had breaking changes, but the public API did not. Among many other libraries, React Query does not include Types when determining semver changes. Since TypeScript is an extremely strict compilation language, any and all type changes would technically be considered breaking changes, which obviously wouldn't work.

As a TypeScript user, you should probably have your dependencies locked down to specific patch versions x.x.patch, and not auto-resolving versions like x.x^. Otherwise, type changes would always break your code without your consent.

With that said, type changes are handled as type: or fix: semantic changes and results in new patch versions. You can and should be manually vetting and upgrading (and consequently ensuring type safety throughout) your application when you upgrade dependencies in TypeScript.

@jackmellis
Copy link
Contributor

Oh man I kinda understand your explanation but I can't tell you how much work this has created for us!

We have react query as a peer dependency in about 20 repos, where 15 of them were written during 2.7 and the others 2.8+. Trying to use them all in a single application has caused hundreds of ts errors.

Errors that arise due to you tightening up an existing type - that's awesome - they almost always shed light on an oversight by the consumer. But when you're not doing breaking changes after renaming an exported type, that's rough! In my books that's totally a change to the public API

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

No branches or pull requests

8 participants