apollographql / apollo-client

:rocket:  A fully-featured, production ready caching GraphQL client for every UI framework and GraphQL server.
https://apollographql.com/client
MIT License
19.38k stars 2.66k forks source link

`mutate` function leaves `data` and `variables` un-typed. #2795

Closed rosskevin closed 5 years ago

rosskevin commented 6 years ago

Intended outcome: apollo-client uses strong typing. This issue is specifically addressing mutate's variables and data.

Actual outcome: The results of mutate are untyped, as are the variables in the options.

How to reproduce the issue: While this issue covers multiple projects' types, the problem is exposed by usage of apollo-client mutate.

apollo-client.mutate is virtually un-typed (e.g. data and variables in typescript). The types that need parameterized adjustments cross apollo-client, apollo-link, and graphql projects. Specifically the data result and variables are unsealed/unchecked and need to be cast if the user wants to use them after fetching.

Summary

  1. data - The mutate<T> from apollo-client doesn't type the result, it types FetchResult<T> position 1, which is the context. The result is in ExecutionResult which is entirely unsealed/untyped data?: { [key: string]: any }
  2. variables - MutationOptions<T> doesn't allow for typing of variables - they are explicitly any
    export interface MutationBaseOptions<T = {
    }> {
    optimisticResponse?: Object | Function;
    updateQueries?: MutationQueryReducersMap<T>;
    refetchQueries?: ((result: ExecutionResult) => RefetchQueryDescription) | RefetchQueryDescription;
    update?: MutationUpdaterFn<T>;
    errorPolicy?: ErrorPolicy;
    variables?: any;
    }
    export interface MutationOptions<T = {
    }> extends MutationBaseOptions<T> {
    mutation: DocumentNode;
    context?: any;
    }
  3. I suggest any parameterized type named T be renamed for clarity to expose issues such as this.

Version

rosskevin commented 6 years ago

Solutions

rosskevin commented 6 years ago

These are roughly the types parameterized, and need to be exposed on mutate:

/**
 * The result of execution. `data` is the result of executing the
 * query, `extensions` represents additional metadata, `errors` is
 * null if no errors occurred, and is a
 * non-empty array if an error occurred.
 */
export interface ExecutionResult<D = Record<string, any>, E = Record<string, any>> {
  data?: D
  extensions?: E
  errors?: GraphQLError[]
}

export declare interface FetchResult<
  D = Record<string, any>,
  C = Record<string, any>,
  E = Record<string, any>
> extends ExecutionResult<D, E> {
  context?: C
}
raysuelzer commented 6 years ago

The workaround seems fine for the time being, because we end up casting anyway in our code.

raysuelzer commented 6 years ago

any update on this?

iamdanthedev commented 6 years ago

Hello

How is it possible that a library that is written in typescript has problems with types? Tools like apollo-codegen provide first class types, but typings for .query and .mutate destroy the benefits of that

Thanks for your hard work anyway :-)

Kocal commented 6 years ago

Hey, is there any updates on this? I would like to help, but I'm not very confortable with TypeScript types definitions. :sweat_smile:

zifahm commented 6 years ago

I am still casting as any for the results ? any update on this?

iamdanthedev commented 6 years ago

I've ended up creating my own wrappers like so:


export type QueryOptions<Result, Variables> = Omit<ApolloQueryOptions, "variables"> & {
  variables: Variables;
};

  return function graphqlQuery<Result, Variables = undefined>(
    options: QueryOptions<Result, Variables>
  ) {
    return client.query<Result>({ ...options, fetchPolicy: "network-only" });
  };
hwillson commented 6 years ago

Most of these suggestions have been implemented across various PR's. If you notice any improvements that could be made to Apollo Client's static typing infrastructure, please feel free to open up a PR. Thanks all!

leebenson commented 6 years ago

@hwillson, there still doesn't seem to be parity between the type of a mutation result returned by client.mutate(), and one executed using the <Mutation> component.

Example:

.data! is a plain object using client.mutate:

screen shot 2018-08-22 at 18 49 53

... but the same types via a <Mutation> yield intellisense:

screen shot 2018-08-22 at 18 52 58
jhalborg commented 6 years ago

+1 for @leebenson 's comment, and friendly ping for @hwillson . It's great that it works with the Mutation component, but in other environments like, say, a Node project, we'll be using client.mutate<> instead, and the latter always returns any.

rosskevin commented 6 years ago

@hwillson please reopen as this is not resolved. Fundamentally ExecutionResult remains untyped.

EDIT: it is now typed in @types/graphl but not passed through in apollo-link:

export declare type FetchResult<C = Record<string, any>, E = Record<string, any>> = ExecutionResult & {
    extensions?: E;
    context?: C;
};

https://github.com/apollographql/apollo-link/blob/2edde34e34ec18241f8732635d62131234129c8b/packages/apollo-link/src/types.ts#L23-L29

The FetchResult above remains invalid generic-wise, it must have an arg for data. Suggested improvement:

export declare interface FetchResult<
  D = Record<string, any>,
  C = Record<string, any>,
  E = Record<string, any>
> extends ExecutionResult<D, E> {
  context?: C
}

Therefore, this needs to be reopened as it is not resolved and apollo-client is not strongly typed.

rosskevin commented 6 years ago

PR for apollo-link up at https://github.com/apollographql/apollo-link/pull/804

rosskevin commented 6 years ago

I have tested the PR locally via augmentation with:

// https://github.com/apollographql/apollo-link/pull/804
// https://github.com/apollographql/apollo-client/issues/2795#issuecomment-424124181

import { ExecutionResult } from 'graphql'

declare module 'apollo-link' {
  export type FetchResult<
    TData = { [key: string]: any },
    C = Record<string, any>,
    E = Record<string, any>
  > = ExecutionResult<TData> & {
    extensions?: E
    context?: C
  }
}

It seems to be working out of the box since apollo-client was trying to pass through the the TData as position 1, albeit errantly as it was applied to C. By adding the above in the PR, it-just-works. Hopefully the apollo-link PR is merged/released.

declanelcocks commented 5 years ago

@rosskevin How were you using the custom typing locally? Your snippet works well, but who knows when the next apollo-link version will be released 😢

rosskevin commented 5 years ago

@declanelcocks look up typescript augmentation.

This issue was fixed and released in apollo-link 1.2.6. PR merged 28 days ago, 1.2.6 released 27 days ago https://github.com/apollographql/apollo-link/releases

alexbjorlig commented 5 years ago

Our team is having issues with this bug, no typings on mutation. We are using apollo-link 1.2.11. Created a stackblitz to illustrate the issue: https://stackblitz.com/edit/apollo-no-typings-on-mutation

@rosskevin are we not expecting typings to work here?

Screenshot 2019-03-28 at 10 21 36

rosskevin commented 5 years ago

@dauledk you need to read the signature on mutate. You must provide the type to it as there is no way to infer it. e.g. mutate<MyType>()

alexbjorlig commented 5 years ago

@rosskevin I updated the code to this.apollo.mutate<{foo: string}>({ but this did not help?

rosskevin commented 5 years ago

Please ask questions on stack overflow. This issue tracker is for bugs/features and there is no need to be notifying watchers of this issue for some help specific to you.

ekron commented 5 years ago

@dauledk Take a look at https://github.com/apollographql/apollo-client/issues/4170#issuecomment-477857406

Might be helpful?