GraphQLSchema => IntrospectionSchema utility

[mjmahone]

The IntrospectionSchema is a useful type for doing things like comparing two versions of the same schema. In order to use it efficiently, this adds a utility for converting from a regular GraphQLSchema directly to an IntrospectionQuery or IntrospectionSchema.

In order to test this effectively against the existing corpus of GraphQLSchema => Introspection, I've also added a utility that takes a raw IntrospectionQuery response, and "cleans it up". This cleaning is necessary, as the response from the server includes fields that aren't explicitly specified in IntrospectionQuery's type system (these fields are always null, such as the interfaces field on types of kind UNION). Furthermore, the response from the server may not include all fields that are specified in the type system. While these fields aren't, strictly speaking, required, it is much easier to work with a fully-and-exactly typed object. Both for tests (where we can use expect().to.deeply.equal() instead of coming up with our own equality matcher) and for general-purpose algorithms and debugging (having many guaranteed-null fields printed in a debugger can cause confusion).

[IvanGoncharov]

@mjmahone You don't need introspectionSchemaFromGraphQLSchema function just use execute for that:

import {execute, parse, getIntrospectionQuery} from 'graphql-js';
const introspection  = await execute({
  schema,
  document: parse(getIntrospectionQuery()),
});

The only one problem I see with this approach is that it's async so you need to deal with promise 😞

[mjmahone]

@IvanGoncharov execute seems like a strange thing to use in the context of, for instance, Relay code generation. This also leads to a lot of weird results, such as the resulting IntrospectionSchema being limited to the level of depth described in the query text we're executing. And that we end up getting extra, unnecessary guaranteed-null fields on the newly-created Introspection types. There's no need to go through the whole stack of a server execution mechanism just to convert the two schema definitions between each other, when that process is very efficient to do synchronously.

I do think it would be good to make clear that this is not meant to be used as part of the GraphQL server execution context, though.

[leebyron]

I agree with @IvanGoncharov - introspection is designed to be collected via execution. The GraphQL execution is lightweight and general purpose for exactly this reason. I don't think the query depth or null fields are weird, they're part of the definition of introspection. This feels like premature optimization to me, what do we gain that we didn't have before?

[leebyron]

The only one problem I see with this approach is that it's async so you need to deal with promise

Right now this is just API simplification. If gaining sync execution is valuable, then we could enable that for all synchronously executable services by having execute not wrap executeOperation in a Promise wrapper. I'd prefer that approach if this is the primary reason for the PR

[leebyron]

For the "cleaning" utility - could this be a simpler general purpose utility that just deletes fields with null values instead of being specific to introspection?

I'm not totally sure what the purpose of this cleaning is since we're typically not calling hasOwnProperty on the results, and it's safe to ignore null values.

If having exact types is the intent, then we could make the existing introspection flow types exact by including the fact that the unrelated fields of __Type are always null for each kind.

[IvanGoncharov]

If gaining sync execution is valuable, then we could enable that for all synchronously executable services by having execute not wrap executeOperation in a Promise wrapper. I'd prefer that approach if this is the primary reason for the PR

Sync execution is very valuable on its own, it will help a lot with GraphQL adoption for many scenarios beyond classic client-server APIs, e.g. graphql-sync. If it falls outside of scope for this PR it would be great to have a separate issue for it to encourage community contribution.

[mjmahone]

@leebyron there was no way to run the test for this correctly against the async/execute query without doing the cleaning: chai doesn't have a comparison that is expect(everythingInHere).to.be.containedIn(expectedThatHasExtraKeys)

It's not good enough to just clean up null values: null, for many of the fields, has semantic meaning. But for many other of them, it doesn't.

[mjmahone]

I'm not going to figure out how to get sync execution working, and I need this internally to use with the next release. So I guess I can close this and just pull the code to be internal-to-facebook instead.

The primary reason for this was to provide a standard "convert from one to the other" API (i.e. something like buildClientSchema(fromIntrsopectionSchema(buildClientSchema(introspectionQuery))). If that's not wanted, that's OK: I want it for my own codebases, so I'll keep it there instead. I'm going to be creating a few utilities that rely heavily on Introspection as a concept (as an isomorphic but better-for-certain-use-cases data model compared to GraphQLSchema), not as a "shape returned from a server execution", and wanted the tools to fit that mental model. (await graphql(schema, introspectionQuery)).data is a pretty strange API that doesn't, IMO, match.

[leebyron]

#1115 adds a sync path for execution - @IvanGoncharov I'd like your opinion of if this will cover the more general cases you've encountered.

[leebyron]

there was no way to run the test for this correctly against the async/execute query without doing the cleaning: chai doesn't have a comparison that is

If the only use case is for writing better tests, then there are other more simple avenues we could take. For example, this codebase uses chai-subset to solve the problem of writing a test where you want to ignore fields you don't care about.

[leebyron]

(await graphql(schema, introspectionQuery)).data is a pretty strange API that doesn't, IMO, match.

That is the specified definition of introspection. If we'd like a nominally easier to understand API, we can certainly add that, but it should be a one-liner implemented using exactly that line of code. I think #1115 could help make that easier by guaranteeing a sync result and at least avoiding async when it's not necessary

[mjmahone]

Yup, this probably isn't useful for now.