More strict typing for Introspection Schema types #1082
Conversation
leebyron
left a comment
leebyron
left a comment
There was a problem hiding this comment.
This is really great. Excellent improvements to these types that better encode the guarantees the spec requires.
Some suggestions inline for further improvements
src/utilities/buildClientSchema.js
Outdated
| // 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 { |
There was a problem hiding this comment.
I don't think moving this to a type argument should be necessary since the T variable isn't used anywhere other than here.
There was a problem hiding this comment.
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.
src/utilities/introspectionQuery.js
Outdated
| directives: Array<IntrospectionDirective>; | ||
| }; | ||
| export type IntrospectionQuery = {| | ||
| +__schema: IntrospectionSchema |
There was a problem hiding this comment.
Could you add a ; to the end of this line while you're at it?
| | IntrospectionEnumType | ||
| | IntrospectionInputObjectType; | ||
|
|
||
| export type IntrospectionScalarType = {| |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
src/utilities/introspectionQuery.js
Outdated
| +kind: 'OBJECT'; | ||
| +name: string; | ||
| +description: ?string; | ||
| +fields: Array<IntrospectionField>; |
There was a problem hiding this comment.
$ReadOnlyArray<> while we're at it?
There was a problem hiding this comment.
Ooh forgot about that one
src/utilities/introspectionQuery.js
Outdated
| +ofType?: T, | ||
| |}; | ||
|
|
||
| export type IntrospectionNonNullTypeRef<T> = {| |
There was a problem hiding this comment.
<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
src/utilities/introspectionQuery.js
Outdated
| }; | ||
| | IntrospectionNamedTypeRef<IntrospectionType> | ||
| | IntrospectionListTypeRef<IntrospectionTypeRef> | ||
| | IntrospectionNonNullTypeRef<IntrospectionTypeRef>; |
There was a problem hiding this comment.
If you take advice above, these definitions don't need to change
src/utilities/introspectionQuery.js
Outdated
| | IntrospectionListTypeRef<IntrospectionInputTypeRef> | ||
| | IntrospectionNonNullTypeRef<IntrospectionInputTypeRef>; | ||
|
|
||
| export type IntrospectionNamedTypeRef<T> = {| |
There was a problem hiding this comment.
Similar to advice above:
<T: IntrospectionType = IntrospectionType>
That avoids a breaking change and offers better types
src/utilities/introspectionQuery.js
Outdated
| export type IntrospectionOutputTypeRef = | ||
| | IntrospectionNamedTypeRef<IntrospectionOutputType> | ||
| | IntrospectionListTypeRef<IntrospectionOutputTypeRef> | ||
| | IntrospectionNonNullTypeRef<IntrospectionOutputTypeRef>; |
There was a problem hiding this comment.
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
3 participants
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 askind: string. This lets us do better, exhaustive, type-safe switches. It also allowsofTypeto no longer need anany-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
IntrospectionInputTypeRefvs.IntrospectionOutputTypeRef.IntrospectionTypeRefis now the superset ofIntrospectionInputTypeRefandIntrospectionOutputTypeRef. This also allows us to make theIntrospectionListTypeRefandIntrospectionNonNullTypeRefmore strict, by passing in, specifically, which set of types they are composing.