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

[graphql] [@apollo/client] add graphql libdef and integrate with @apollo/client #4550

Merged
merged 31 commits into from Nov 6, 2023
Merged

[graphql] [@apollo/client] add graphql libdef and integrate with @apollo/client #4550

merged 31 commits into from Nov 6, 2023

Conversation

joshuanapoli
Copy link
Contributor

@joshuanapoli joshuanapoli commented Nov 4, 2023

  • Type of contribution: add libdef

Other notes:

  • Add a module definition for "@apollo/client/utilities". This module has a CJS export and is meant as a public interface of the package, though not thoroughly documented. Moreover, not all of its symbols are exported at the root level; importing the sub-module in an application is necessary in some cases. In my case, I need getMainDefinition, which is shown in apollo client example code.
  • Fix ApolloLink and Operation class inheritance, which were incorrectly extending a global class. This didn't create the correct class inheritance and caused wrong flow errors in correctly written application code.
  • Move some hack global types back into their proper module, to match the typescript sources.

There is a limitation in flow libdefs, which makes it awkward to define a class hierarchy that spans a module boundary. The following is an error:

declare module "@apollo/client/link/core" {
  declare export class ApolloLink {};
}

declare module "@apollo/client" {
  import type { ApolloLink } from "@apollo/client/link/core";
  declare export class HttpLink extends ApolloLink {}; // value-as-type error

I tried combinations of import typeof and extends Class<Base>, but these don't seem to solve the problem.

In this PR I left the error suppression $FlowFixMe[value-as-type] in the libdef. This lets the libdef source express the class hierarchy and module membership correctly to a human reader, and there are no incorrect flow diagnostics in application code that uses these types.

// @apollo/client/link/core/types.d.ts
// Globals to support global ApolloLink class
declare type ApolloClient$Path = $ReadOnlyArray<string | number>;
declare type GraphQL$VariableNode = Object;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't use Object as this resolves to any. Use { ... } or { [key: string]: any } instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 93a5212

In this case, I added a libdef for "graphql", because { ... } or { [key: string]: any } led to flow errors in correct application code around the GraphQL types.

}
declare export function makeReference(id: string): Reference;
declare export function isReference(obj: any): boolean;
declare export type StoreValue = number | string | string[] | Reference | Reference[] | null | void | void | Object;
Copy link
Member

Choose a reason for hiding this comment

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

Should tidy this type up a bit. void | void and Object should be { ... } as mentioned above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in b3c6310

argObj: any,
name: GraphQL$NameNode,
value: GraphQL$ValueNode,
variables?: Object
Copy link
Member

Choose a reason for hiding this comment

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

Object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in a763180

@Brianzchen
Copy link
Member

Re ApolloLink issue. Could you declare the class in the top level and then export/use in the two module definitions?

@joshuanapoli joshuanapoli changed the title [@apollo/client] fix Operation and HttpLink class inheritance [graphql] [@apollo/client] add graphql libdef and integrate with @apollo/client Nov 5, 2023
@joshuanapoli
Copy link
Contributor Author

joshuanapoli commented Nov 5, 2023

Re ApolloLink issue. Could you declare the class in the top level and then export/use in the two module definitions?

One problem with with this approach is that import type is an error at the top level of a library. I would like to use imported types (from "graphql" and "@apollo/client/...") in the ApolloLink definition.

@Brianzchen
Copy link
Member

Re ApolloLink issue. Could you declare the class in the top level and then export/use in the two module definitions?

One problem with with this approach is that import type is an error at the top level of a library. I would like to use imported types (from "graphql" and "@apollo/client/...") in the ApolloLink definition.

Right I see

@Brianzchen Brianzchen merged commit 7599864 into flow-typed:main Nov 6, 2023
4 checks passed
@joshuanapoli joshuanapoli deleted the apollo/client/Operation branch November 6, 2023 02:54
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

2 participants