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

Regression in 7.4.1: TData generic breaks type check for custom mutation options function #1820

Closed
severinh opened this issue Jan 13, 2025 · 12 comments · Fixed by #1871
Closed
Assignees
Labels
fetch Fetch client related issue tanstack-query TanStack Query related issue
Milestone

Comments

@severinh
Copy link
Contributor

severinh commented Jan 13, 2025

What are the steps to reproduce this issue?

In our system, we use the mutationOptions Orval configuration to add a default error handler to all mutations to show an error snackbar in the product (unless overridden by the user of the mutation):

Config used

override: {
  query: {
    mutationOptions: {
      name: 'useDefaultOpenApiMutationOptions',
      path: 'src/common/hooks/useDefaultOpenApiOptions.ts',
    },
  }
}

useDefaultOpenApiOptions.ts contains the following:

/** Shows a default, generic error snack bar if the request fails. Callers can override this behavior. */
export const useDefaultOpenApiMutationOptions = <
  TData = unknown, // All unknown because this is function is used across all mutations.
  TError = unknown,
  TVariables = void,
  TContext = unknown,
>(
  mutationOptions: UseMutationOptions<TData, TError, TVariables, TContext>
): UseMutationOptions<TData, TError, TVariables, TContext> => {
  return {
    onError: (error) => {
      SnackbarUtils.error('Something went wrong');
    },
    ...mutationOptions,
  };
};

This worked well until 7.3.0. With the upgrade to 7.4.0, the code generated by Orval does not type-check anymore.

image

The error is:

Type 'MutationFunction<CreateOfferImportResponse, { roundId: string; data: SupplierUpdateOffersRequest; }>' is not assignable to type 'MutationFunction<TData, { roundId: string; data: SupplierUpdateOffersRequest; }>'.
  Type 'CreateOfferImportResponse' is not assignable to type 'TData'.
    'TData' could be instantiated with an arbitrary type which could be unrelated to 'CreateOfferImportResponse'.ts(2322)
types.d.ts(354, 5): The expected type comes from property 'mutationFn' which is declared here on type 'UseMutationOptions<TData, TError, { roundId: string; data: SupplierUpdateOffersRequest; }, TContext>'

The same type check error happens for all generated mutations.

Cause

The change that introduced this breakage is #1735.

The comment in that PR mentions that this is a breaking change, but it does not say how teams affected by the breakage could resolve the breakage.

Resolution?

I could not figure out how to update our custom mutation options function to make the type checking issue go away. It seems the problem is not our function, but rather the type inconsistency in the generated mutation itself between the mutationOptions variable (any TData allowed) and mutationFn (TData is the actual response type).

Could you tell me how I can make this type-check? Right now this blocks us from upgrading to Orval 7.4.0.

If it's not possible on our end, can we revert #1735? I personally don't understand why that PR was needed, and what value it brings to override the TData of a mutation.

What versions are you using?

  System:
    OS: macOS 14.6.1
    CPU: (10) arm64 Apple M1 Pro
    Memory: 405.61 MB / 32.00 GB
    Shell: 5.9 - /bin/zsh
  npmPackages:
    @tanstack/react-query: 4.36.1 => 4.36.1 
    axios: 1.7.9 => 1.7.9 
    msw: 2.6.8 => 2.6.8 
    orval: 7.4.1 => 7.4.1 
    react: 18.3.1 => 18.3.1 
    zod: 3.23.8 => 3.23.8 
@melloware
Copy link
Collaborator

Can you try 7.4.1 i think this is fixed.

@melloware melloware added this to the 7.4.1 milestone Jan 13, 2025
@melloware melloware added the bug Something isn't working label Jan 13, 2025
@severinh
Copy link
Contributor Author

severinh commented Jan 13, 2025

Can you try 7.4.1 i think this is fixed.

Thanks a lot for the quick reply! I'm afraid not. Both 7.4.0 and 7.4.1 have the same problem.

I've in the meantime I found a way to unblock the upgrade by dropping the type safety in the custom mutation options function. Something like the following:

/** Shows a default, generic error snack bar if the request fails. Callers can override this behavior. */
export const useDefaultOpenApiMutationOptions = <T>(mutationOptions: T): T => {
  const { t } = useTranslation();
  // The parameter has the type UseMutationOptions. However, using this type would cause the call
  // in the Orval-generated mutation code to not type-check due to https://github.com/orval-labs/orval/issues/1820.
  // Hence, for loosen the type safety of the method signature and cast.
  const typedMutationOptions = mutationOptions as UseMutationOptions;
  return {
    onError: (error: unknown) => {
      // Only display snackbar if error is not being caught by an error boundary.
      if (!typedMutationOptions.useErrorBoundary) {
        SnackbarUtils.error('Something went wrong');
      }
    },
    ...typedMutationOptions,
  } as T;
};

While this works, it's essentially a hack. I think other users of Orval will run into the same problem when trying to use custom mutation options functions, and will not know how to type them. Orval's documentation does not give an example of how to type such a function.

@melloware melloware modified the milestones: 7.4.1, 7.4.2 Jan 13, 2025
@melloware melloware added tanstack-query TanStack Query related issue and removed bug Something isn't working labels Jan 13, 2025
@oferitz-apex
Copy link

oferitz-apex commented Jan 14, 2025

I think it's not just with override of mutation options but a more broad issue with override in general.
I'm using a mutator override with custom fetch:
Screenshot 2025-01-14 at 9 27 07

And these new union with void types started to appear:
Screenshot 2025-01-14 at 9 25 26

Which breaks the inference of the type where i want to use it:
Screenshot 2025-01-14 at 9 25 53

@melloware melloware added the fetch Fetch client related issue label Jan 14, 2025
@zdarovka
Copy link

I also think that since the 7.4 the support of partialOptions was broken. All of the queries now do not have the Partil options generated in the client

https://github.com/lentyaevpk/orval/blob/master/packages/query/src/index.ts#L501

@melloware
Copy link
Collaborator

@zdarovka did you try 7.4.1?

@zdarovka
Copy link

yes

@melloware melloware changed the title Regression in 7.4.0: TData generic breaks type check for custom mutation options function Regression in 7.4.1: TData generic breaks type check for custom mutation options function Jan 22, 2025
@melloware
Copy link
Collaborator

OK I updated the title of the ticket to say 7.4.1

@soartec-lab
Copy link
Member

I was thinking explicit typing would be preferable, but does that mean we lose type safety if we use custom mutations?
This is not what I intended, so I think I can revert #1735.

@lentyaevpk

What do you think?

@soartec-lab soartec-lab self-assigned this Feb 1, 2025
@soartec-lab
Copy link
Member

@severinh @oferitz-apex @zdarovka @melloware

Hey guys,
from @lentyaevpk No response, but I'm going to create a PR that reverts this. What do you all think?

@oferitz
Copy link

oferitz commented Feb 1, 2025

@soartec-lab In my specific case the union type with void is breaking the auto type inference, so if the revert will fix it i'm definitely support it.

@melloware
Copy link
Collaborator

Yep go ahead and revert it.

@soartec-lab
Copy link
Member

@severinh @oferitz-apex @zdarovka @melloware

I reverted this by #1871 . A new version will be released soon, so please check back soon. Look forward to the new version.

@oferitz
Your issue may not be fixed by this solution. If the problem recurs in the next release, would you please open a new issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fetch Fetch client related issue tanstack-query TanStack Query related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants