More strict typing for Introspection Schema types by mjmahone · Pull Request #1082 · graphql/graphql-js

mjmahone · 2017-11-03T18:49:14Z

Flow has added the ability to have read-only properties. Given, once an introspection query's result is available, it's effectively read-only, I decided to encode that into the type system.

Additionally, we can make objects "strict" now: introspection types can't have more fields than are defined on them via the type system, so I added strictness to each Introspection type.

Furthermore, flow also introduced some utility types, including $PropertyType. This type allows us to force a type reference's "kind" property to match a specific other type's "kind", rather than typing it more generically as kind: string. This lets us do better, exhaustive, type-safe switches. It also allows ofType to no longer need an any-escape-hatch.

Finally, in many cases there is a distinction between Input and Output types: for instance, field arguments can only be Input types. With the above changes, it was fairly easy to encode this distinction as IntrospectionInputTypeRef vs. IntrospectionOutputTypeRef. IntrospectionTypeRef is now the superset of IntrospectionInputTypeRef and IntrospectionOutputTypeRef. This also allows us to make the IntrospectionListTypeRef and IntrospectionNonNullTypeRef more strict, by passing in, specifically, which set of types they are composing.

… possible

leebyron

This is really great. Excellent improvements to these types that better encode the guarantees the spec requires.

Some suggestions inline for further improvements

leebyron · 2017-11-03T20:22:30Z

  // Given a type reference in introspection, return the GraphQLType instance.
  // preferring cached instances before building new instances.
-  function getType(typeRef: IntrospectionTypeRef): GraphQLType {
+  function getType<T: IntrospectionTypeRef>(typeRef: T): GraphQLType {


I don't think moving this to a type argument should be necessary since the T variable isn't used anywhere other than here.

This is useful to end up with more-restrictive types: if I explicitly pass in a IntrospectionOutputTypeRef I could change this to return a GraphQLOutputType at some point in the future. Though for now you're probably right that this is unnecessary (and we may want to just make that a different function).

Also I didn't realize this was an internal-only-function so even more unnecessary :) Will change.

leebyron · 2017-11-03T20:24:30Z

-  directives: Array<IntrospectionDirective>;
-};
+export type IntrospectionQuery = {|
+  +__schema: IntrospectionSchema


Could you add a ; to the end of this line while you're at it?

leebyron · 2017-11-03T20:31:24Z

+  | IntrospectionEnumType
+  | IntrospectionInputObjectType;
+
+export type IntrospectionScalarType = {|


I'm not sure if these types should be exact types since that doesn't actually describe what is returned from the server - all fields are queried on each possible type, so the properties exist and are null.

That might be okay as just a balance between correctness and usability, but we should keep an eye on this in case we need to remove the exact types in the future.

I'd prefer exact types for now, as it's way easier to pretend fields that are always null don't exist. If we have to add a bunch of null fields, we might want a conversion from IntrospectionType to LinkedType to give us the more-restrictive typing (could also do things like enforce ofType is non-null on List and NonNullable).

Basically, if we need to relax this, then we probably want two different sets of types:

IntrospectionType => things coming over-the-wire in an introspection query. Does not differentiate from Input and Output types, but is a 1:1 data representation.

LinkedType => type system description using reference types that is a validated version of what came down from the wire.

I think it's fine to keep it exact types for now - just wanted to highlight the disconnect so we're aware in the future

leebyron · 2017-11-03T20:31:50Z

+  +kind: 'OBJECT';
+  +name: string;
+  +description: ?string;
+  +fields: Array<IntrospectionField>;


$ReadOnlyArray<> while we're at it?

Ooh forgot about that one

leebyron · 2017-11-03T20:32:42Z

+  +ofType?: T,
+|};
+
+export type IntrospectionNonNullTypeRef<T> = {|


<T: IntrospectionType = IntrospectionType>

same above. That means supplying T must be a subtype of IntrospectionType, but you can also omit , which just implies IntrospectionType - this is nice because it means this won't be a breaking change for anyone already using these types

leebyron · 2017-11-03T20:35:33Z

-};
+  | IntrospectionNamedTypeRef<IntrospectionType>
+  | IntrospectionListTypeRef<IntrospectionTypeRef>
+  | IntrospectionNonNullTypeRef<IntrospectionTypeRef>;


If you take advice above, these definitions don't need to change

leebyron · 2017-11-03T20:36:33Z

+  | IntrospectionListTypeRef<IntrospectionInputTypeRef>
+  | IntrospectionNonNullTypeRef<IntrospectionInputTypeRef>;
+
+export type IntrospectionNamedTypeRef<T> = {|


Similar to advice above:

<T: IntrospectionType = IntrospectionType>

That avoids a breaking change and offers better types

leebyron · 2017-11-03T20:38:34Z

+export type IntrospectionOutputTypeRef =
+  | IntrospectionNamedTypeRef<IntrospectionOutputType>
+  | IntrospectionListTypeRef<IntrospectionOutputTypeRef>
+  | IntrospectionNonNullTypeRef<IntrospectionOutputTypeRef>;


This is technically not correct since a non-null cannot wrap a non-null. Elsewhere where we encode types that account for list and non-null we account for this. It might look something like:

export type IntrospectionOutputTypeRef = | IntrospectionNamedTypeRef<IntrospectionOutputType> | IntrospectionListTypeRef<IntrospectionOutputTypeRef> | IntrospectionNonNullTypeRef< | IntrospectionNamedTypeRef<IntrospectionOutputType> | IntrospectionListTypeRef<IntrospectionOutputTypeRef> >;

Similar below

leebyron

Solid improvements!

mjmahone added 4 commits November 3, 2017 14:24

Make Introspection types read-only, split Input vs Output types where…

552cf19

… possible

Lint fixes

82e1533

NonNull and List keep their TypeRef names

07235a1

Remove typehints I was using to track down flow failure

983160a

facebook-github-bot added the CLA Signed label Nov 3, 2017

mjmahone requested review from kassens and leebyron November 3, 2017 18:56

leebyron requested changes Nov 3, 2017

View reviewed changes

Make suggested changes

8026622

leebyron approved these changes Nov 4, 2017

View reviewed changes

leebyron merged commit 2419c3d into graphql:master Nov 4, 2017

This was referenced Apr 24, 2021

[Snyk] Upgrade cspell from 5.2.4 to 5.3.10 turkdevops/graphql-js#148

Closed

[Snyk] Upgrade cspell from 5.2.4 to 5.3.11 turkdevops/graphql-js#149

Closed

snyk-bot mentioned this pull request May 4, 2021

[Snyk] Upgrade cspell from 5.2.4 to 5.3.12 turkdevops/graphql-js#157

Closed

Conversation

mjmahone commented Nov 3, 2017

Uh oh!

leebyron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leebyron left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants