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

[@types/graphql] Update to v0.13.2 #24566

Merged
merged 13 commits into from Apr 3, 2018

Conversation

Projects
None yet
8 participants
@firede
Contributor

firede commented Mar 27, 2018

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: https://github.com/graphql/graphql-js/tree/v0.13.2
  • Increase the version number in the header if appropriate.
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.
@firede

This comment has been minimized.

Contributor

firede commented Mar 27, 2018

The main work has been completed 🎉

This version is ready for strictNullChecks, although I haven't enabled it in tsconfig yet.

However, there are still some dependent packages can't pass unit tests.
As a related issue, I will submit some separate PRs for them. (done, waiting for merge.)

Compatibility test

📝TODO:

  • graphql-tools see below
  • apollo-server-core see below
  • apollo-client see below

Errors

graphql-list-fields #24582 (merged)

types/graphql-list-fields/graphql-list-fields-tests.ts

const sampleGraphQLResolveInfo: GraphQLResolveInfo = {
    fieldName: "",
    fieldNodes: [],
    returnType: GraphQLString,
    parentType: new GraphQLObjectType({
        name: "Sample",
        fields: {
            name: { type: GraphQLString }
        }
    }),
    path: undefined,
//        ^ Type 'undefined' is not assignable to type 'ResponsePath'.

Definition:

  +path: ResponsePath, // required
  +schema: GraphQLSchema,
  +fragments: ObjMap<FragmentDefinitionNode>,
  +rootValue: mixed,
  +operation: OperationDefinitionNode,
  +variableValues: { [variable: string]: mixed },
|};

export type ResponsePath = {|
  +prev: ResponsePath | void,
  +key: string | number,
|};

graphql-relay #24535 (merged)

types/graphql-relay/graphql-relay-tests.ts

ERROR: 47:10   expect  TypeScript@next compile error:
Cannot assign to 'fieldName' because it is a constant or a read-only property.
ERROR: 122:10  expect  TypeScript@next compile error:
Cannot assign to 'fieldName' because it is a constant or a read-only property.

Definition:

export type GraphQLResolveInfo = {|
  +fieldName: string, // readonly

express-graphql #24581 (merged)

types/express-graphql/express-graphql-tests.ts

Type '{ getQueryType: null; getMutationType: null; getSubscriptionType: null; getTypeMap: null; getType...' is not assignable to type 'GraphQLSchema'.
  Property 'astNode' is missing in type '{ getQueryType: null; getMutationType: null; getSubscriptionType: null; getTypeMap: null; getType...'.

Definition:

export class GraphQLSchema {
  astNode: ?SchemaDefinitionNode; // required

Related PR: #24532 #24552

@typescript-bot

This comment has been minimized.

typescript-bot commented Mar 27, 2018

@firede Thank you for submitting this PR!

🔔 @TonyPythoneer @calebmer @intellix @kepennar @freiksenet @IvanGoncharov @DxCx @rportugal @tgriesser @dyst5422 @adnsio - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@typescript-bot

This comment has been minimized.

typescript-bot commented Mar 27, 2018

@firede The Travis CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks!

firede added a commit to firede/DefinitelyTyped that referenced this pull request Mar 28, 2018

Defining `schema` the right way.
Otherwise, it will cause the new version `@types/graphql` test to fail.
Related: DefinitelyTyped#24566

firede added a commit to firede/types-graphql that referenced this pull request Apr 1, 2018

@firede

This comment has been minimized.

Contributor

firede commented Apr 1, 2018

graphql-tools changed some read-only props.
It works well, but doesn't match the type definitions.

Like this:

image

If graphql-tools upgrade @type/graphql to 0.13.x, there will be many errors.

@IvanGoncharov

This comment has been minimized.

Contributor

IvanGoncharov commented Apr 1, 2018

graphql-tools changed some read-only props.
If graphql-tools upgrade @type/graphql to 0.13.x, there will be many errors.

@firede Yes, it should be fixed in grahql-tools by adding explicit casts before updating to a new @type/graphql.

Ping: @stubailo @benjamn

@benjamn

This comment has been minimized.

benjamn commented Apr 2, 2018

@firede @IvanGoncharov Would either of you have a minute to submit a PR to fix that read-only reassignment problem? An explicit cast seems 👌 .

@IvanGoncharov

This comment has been minimized.

Contributor

IvanGoncharov commented Apr 2, 2018

@benjamn I'm not using any of Apollo products/projects. So no for me.
Just wanted to notify you of a possible problem with your code.
I think you can easily find all such places and fix them once this PR get merged.

@IvanGoncharov

This comment has been minimized.

Contributor

IvanGoncharov commented Apr 3, 2018

@firede 👍 from me.
Thanks a lot for the huge amount of work.

@typescript-bot

This comment has been minimized.

typescript-bot commented Apr 3, 2018

A definition author has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

@benjamn

This comment has been minimized.

benjamn commented Apr 3, 2018

@firede Were you able to generate the types automatically from the graphql-js repository somehow, or was this a lot of manual work for you? It would be great if we had tools for preventing the mismatch going forward (if possible)!

@firede

This comment has been minimized.

Contributor

firede commented Apr 3, 2018

There are no complete tools to convert flow to typescript yet, and I don't have time to write one.
I tried flow-to-typescript and tsgen, but the results were not ideal.

Some difficulties

Flow's Utility Types is hard to convert automatically. e.g.

Flow
export type ASTVisitor = Visitor<ASTKindToNode>;
export type Visitor<KindToNode, Nodes = $Values<KindToNode>> =
  | EnterLeave<
      | VisitFn<Nodes>
      | ShapeMap<KindToNode, <Node>(Node) => VisitFn<Nodes, Node>>,
    >
  | ShapeMap<
      KindToNode,
      <Node>(Node) => VisitFn<Nodes, Node> | EnterLeave<VisitFn<Nodes, Node>>,
    >;
type EnterLeave<T> = {| +enter?: T, +leave?: T |};
type ShapeMap<O, F> = $Shape<$ObjMap<O, F>>;
TypeScript

This version works, but it still needs improvement.

interface EnterLeave<T> {
    readonly enter?: T;
    readonly leave?: T;
}
type EnterLeaveVisitor<KindToNode, Nodes> = EnterLeave<
    VisitFn<Nodes> | { [K in keyof KindToNode]?: VisitFn<Nodes, KindToNode[K]> }
>;
type ShapeMapVisitor<KindToNode, Nodes> = {
    [K in keyof KindToNode]?: VisitFn<Nodes, KindToNode[K]> | EnterLeave<VisitFn<Nodes, KindToNode[K]>>
};
export type ASTVisitor = Visitor<ASTKindToNode>;
export type Visitor<KindToNode, Nodes = KindToNode[keyof KindToNode]> =
    | EnterLeaveVisitor<KindToNode, Nodes>
    | ShapeMapVisitor<KindToNode, Nodes>;

Where javascript and flow mix, probably need some tricks:

Flow
export const DirectiveLocation = Object.freeze({
  QUERY: 'QUERY',
  MUTATION: 'MUTATION',
  // ...
  INPUT_FIELD_DEFINITION: 'INPUT_FIELD_DEFINITION',
});

export type DirectiveLocationEnum = $Values<typeof DirectiveLocation>;
TypeScript
export const DirectiveLocation: _DirectiveLocation;
type _DirectiveLocation = {
    QUERY: "QUERY";
    MUTATION: "MUTATION";
    // ...
    INPUT_FIELD_DEFINITION: "INPUT_FIELD_DEFINITION";
};

export type DirectiveLocationEnum = _DirectiveLocation[keyof _DirectiveLocation];

@benjamn You guessed right, it was a lot of boring manual work. :-)

@paulvanbrenk paulvanbrenk merged commit 1038a9a into DefinitelyTyped:master Apr 3, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@typescript-bot typescript-bot removed this from Merge: Express in Pull Request Triage Backlog Apr 3, 2018

@firede firede deleted the firede:graphql-v0.13.2 branch Apr 11, 2018

@bradzacher

This comment has been minimized.

Contributor

bradzacher commented May 2, 2018

I see in this version there was a lot of types that changed from being | undefined to | void.

Why was this change made?
This has made a lot of the code much harder to use in practice.

For example:

function oldGetField(name : string, schema : GraphQLSchema) {
	const field = schema.getQueryType().getFields()[name]
	//                                /\
    // no longer works ---------------|
    // getQueryType returns | void, so there is no way to chain the methods.
}

function newGetField(name : string, schema : GraphQLSchema) {
	const queryType = schema.getQueryType()
	if (queryType) {
		const field = queryType.getFields()[name]
	}
}

void implies that that the function doesn't return anything, which is technically the same as undefined, but the two are treated very differently in typescript.

The example function is flow typed as ?GraphQLObjectType (which is a Maybe type in flow - the same as saying | null | undefined in TS), which does not match the | void type.

@firede

This comment has been minimized.

Contributor

firede commented May 2, 2018

@bradzacher That's an interesting issue, probably have different opinions.

First, explain why I use void.

  • Flow has built-in Maybe types.
  • From TypeScript 1.8, you can represent type Maybe<T> = T | void.

Technically:

  • void in Flow types equals to undefined.
  • void in TypeScript equals to null | undefined.
// Flow
type Foo = {
  a: ?string
  b?: ?number
}

// TypeScript

interface Foo {
  // Absolutely, `null | undefined` is correct.
  a: string | null | undefined
  b?: number | null
}

interface Foo {
  // As the semantics of `Maybe`, `void` can express
  // the correct meaning and be more simplified.
  a: string | void
  b?: number | null
}

Back to the example:

function getField(name: string, schema: GraphQLSchema) {
  // Sometimes the query type does not exist, e.g.
  //
  //   const schema = buildSchema(`type Foo { bar: String! }`)
  //   schema.getQueryType() // => null
  //
  // If you could guarantee that the query type exists,
  // you can chain the methods by this way:
  const field = (schema.getQueryType() as GraphQLObjectType).getFields()[name]
}
@bradzacher

This comment has been minimized.

Contributor

bradzacher commented May 2, 2018

The reason I say that it would be better to use | null | undefined instead of | void is because there is the inbuilt operator for removing those types, the ! type operator:

function getField(name: string, schema: GraphQLSchema) {
  const field = schema.getQueryType()!.getFields()[name]
}

In the case of my company's API - we have utilities setup to run only on query entities during a resolve, so we can guarantee that getQueryType returns a schema. It's a bit cumbersome to have to do a typecast every time instead of using the ! operator.

Additionally - using undefined means that the code works fine with strictNullChecks off, and it won't compile with it on. However void will fail regardless of the setting.

@firede

This comment has been minimized.

Contributor

firede commented May 2, 2018

👍 You are right, null | undefined is better.

Could you please submit a pull request to fix it?
Or I deal with this later (I am currently doing other work).

@bradzacher

This comment has been minimized.

Contributor

bradzacher commented May 2, 2018

Happy to! 👍
I'll give it a shot first thing tomorrow morning (it's late here in AU).

@bradzacher

This comment has been minimized.

Contributor

bradzacher commented May 9, 2018

Took me a bit longer to get to than I wanted, but it's done :) #25639

@sujeshthekkepatt

This comment has been minimized.

sujeshthekkepatt commented May 14, 2018

hey is this PR out ? iam only able to install 0.13.1!!!

@bradzacher

This comment has been minimized.

Contributor

bradzacher commented May 14, 2018

both this pr and #25639 have been merged and released.
The latest version is 0.13.1.

The version of the typings package does not exactly match that of the graphql package.
typings v0.13.x should be for graphql v0.13.y.
Note that it not necessary that x and y are not necessarily the same.

@sujeshthekkepatt

This comment has been minimized.

sujeshthekkepatt commented May 14, 2018

ok but I am getting "Type 'GraphQLSchema' cannot be converted to type 'GraphQLSchema'.
Property 'astNode' is missing in type 'GraphQLSchema'" error. what about that?

@bradzacher

This comment has been minimized.

Contributor

bradzacher commented May 14, 2018

because you have mismatched versions of @types/graphql.

Try rm -fr node_modules && npm install.

@sujeshthekkepatt

This comment has been minimized.

sujeshthekkepatt commented May 14, 2018

But the issue persists. I have updated to same version but yet it exists. graphql@0.13.1 and @types/graphql at 0.13.1

@firede

This comment has been minimized.

Contributor

firede commented May 14, 2018

@sujeshthekkepatt
I guess(from the error message), the schema you define like this:

const schema: GraphQLSchema = {
    getQueryType: null,	
    getMutationType: null,	
    getSubscriptionType: null,	
    // ...
};

However, astNode is not a optional property in GraphQLSchema.

export class GraphQLSchema {
  astNode: ?SchemaDefinitionNode;

You better do this through new GraphQLSchema or buildSchema.

@sujeshthekkepatt

This comment has been minimized.

sujeshthekkepatt commented May 14, 2018

@firede Iam creating the schema using 'makeExecutableSchma' function which returns GraphQLSchema type object.

@sujeshthekkepatt

This comment has been minimized.

sujeshthekkepatt commented May 14, 2018

@firede It seems that the issue is with graphql-tools which provide the makeExecutableSchema function. Updated the library and the error is fixed. Thank you for the quick response. Thank you

@glasser

This comment has been minimized.

glasser commented May 17, 2018

Hmm. Before this, most Errors could be treated as (cast to? I'm not a TypeScript expert) GraphQLErrors, because most of the fields were ?. Now those fields lack ?, even though they all are either |undefined or |void. Was this intentional?

This is causing a bit of trouble for upgrading apollo-server.

@firede

This comment has been minimized.

Contributor

firede commented May 18, 2018

@glasser IMHO, @types/graphql should follow the semantics of graphql flow definition.


I checked the error of apollo-server-core, TypeScript report this:

[ts]
Argument of type 'Error' is not assignable to parameter of type 'GraphQLError'.
  Property 'locations' is missing in type 'Error'.
---
const newError: Error

If you use the official GraphQL (flow):

// @flow
const { formatError } = require("graphql");
const newError = new Error("Internal server error");
formatError(newError);

Flow will report an error:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ test.js:4:13

Cannot call formatError with newError bound to error because Error [1] is incompatible
with GraphQLError [2].

     test.js
      1│ // @flow
      2│ const { formatError } = require("graphql");
 [1]  3│ const newError = new Error("Internal server error");
      4│ formatError(newError);
      5│

     node_modules/graphql/error/formatError.js.flow
 [2] 18│ export function formatError(error: GraphQLError): GraphQLFormattedError {

Found 1 error

If you change the code like this (Error -> GraphQLError):

const { formatError, GraphQLError } = require("graphql");
const newError = new GraphQLError("Internal server error");
formatError(newError);

No errors!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment