Add buildASTSchemaComponents function #817

IvanGoncharov · 2017-04-19T16:51:49Z

At the moment you can use IDL only to describe entire Schema.
But in a few our projects we need to build only directives and associated types from IDL.
We work around this by appending following to the IDL:

 type Query {
    dummy: String
 }

This PR splits out the major part of buildASTSchema function code and allows to get types and directives before they are passed to GraphQLSchema constructor.

wincent

Makes a lot of sense! Thanks for this. Some small requests inline.

wincent · 2017-04-21T22:28:23Z

src/utilities/buildASTSchema.js

 */
-export function buildASTSchema(ast: DocumentNode): GraphQLSchema {
+export function buildASTSchemaConfig(ast: DocumentNode) {


I think we should define and specify a return type here (GraphQLSchemaConfig, perhaps?).

@wincent I can't return GraphQLSchemaConfig since it require query property to be not null:

type GraphQLSchemaConfig = { query: GraphQLObjectType; mutation?: ?GraphQLObjectType;

So it should some type identical to GraphQLSchemaConfig but with query defined as:

query: ?GraphQLObjectType;

Can you please suggest best way how to do this in Flow?

Yeah, sorry: I didn't mean literally GraphQLSchemaConfig; I meant some name like it.

In the worst case you define two types with different names. Better though, would be to use an intersection type:

type Base = { mutation?: ?GraphQLObjectType; subscription?: ?GraphQLObjectType; types?: ?Array<GraphQLNamedType>; directives?: ?Array<GraphQLDirective>; }; type A = Base & { query: GraphQLObjectType; }; type B = Base & { query?: GraphQLObjectType; };

(Leaving the actual names of A and B out just for illustration.)

@wincent Thank you. I used "GraphQLSchemaComponents" as the name for the new type and also rename function to buildASTSchemaComponents.

wincent · 2017-04-21T22:28:48Z

src/utilities/buildASTSchema.js

+ * has no resolve methods, so execution will use default resolvers.
+ */
+export function buildASTSchema(ast: DocumentNode): GraphQLSchema {
+  const config:any = buildASTSchemaConfig(ast);


nit: if we were to keep this any (and I don't think we should), we'd want a space after the colon.

@wincent I added space. As for using proper type it's the same problem with typing that I described in my previous comment :(

@wincent I tried to make config: GraphQLSchemaComponents but it sims that Flow don't recognize runtime checks like this one: https://github.com/graphql/graphql-js/pull/817/files#diff-2691b670eb7e51187edc1f918c624a70R145
So when I call new GraphQLSchema(config) flow produce an error saying query is missing. Can you please advice on how to make this possible in Flow?

@wincent I think it last change suggested in your review. Can you please advice how it can be solved in Flow?

wincent · 2017-04-21T22:31:11Z

src/utilities/buildASTSchema.js

@@ -132,10 +132,9 @@ function getNamedTypeNode(typeNode: TypeNode): NamedTypeNode {
 * If no schema definition is provided, then it will look for types named Query
 * and Mutation.
 *
- * Given that AST it constructs a GraphQLSchema. The resulting schema
- * has no resolve methods, so execution will use default resolvers.
+ * Given that AST it return object that can be used to construct GraphQLSchema.


spelling: return → returns

suggest: "construct GraphQLSchema" → "construct a GraphQLSchema instance"

leebyron

I'm curious - if you only expect to parse types and directives and explicitly working around not wanting to provide a schema definition, why also parse the schema definition in this new function?

Also, this is also including the standard built-in types in the return, is that what you need from this function? It's a bit strange that built-in types are added but built-in directives are not.

I also think the naming of this new function is confusing. Is the intent that you would use the response of it to call new GraphQLSchema()? It doesn't sound like it from the PR description. Maybe buildASTDefinitions?

I don't think you need this new function to be called directly by buildASTSchema. I'm sure they can share a majority of implementation via module-private functions, but this particular implementation seems to cripple the intent described at the top of the PR.

leebyron · 2017-05-15T16:50:31Z

src/utilities/buildASTSchema.js

+ * Given that AST it constructs a GraphQLSchema. The resulting schema
+ * has no resolve methods, so execution will use default resolvers.
+ */
+export function buildASTSchema(ast: DocumentNode): GraphQLSchema {


This should be the first function in the file since its eponymous.

Thanks, done.

leebyron · 2017-05-15T16:51:00Z

src/utilities/buildASTSchema.js

+ * has no resolve methods, so execution will use default resolvers.
+ */
+export function buildASTSchema(ast: DocumentNode): GraphQLSchema {
+  const config: any = buildASTSchemaComponents(ast);


Shouldn't need an :any cast here, at least not without clear comment as to why. Seems unsound

@leebyron I tried to make config: GraphQLSchemaComponents but it seems that Flow doesn't recognize runtime checks like this one: https://github.com/graphql/graphql-js/pull/817/files#diff-2691b670eb7e51187edc1f918c624a70R145
So when I call new GraphQLSchema(config), Flow produces an error saying query is missing. Could you please advice on how to make this possible in Flow?

IvanGoncharov · 2017-05-16T11:16:13Z

I'm curious - if you only expect to parse types and directives and explicitly working around not wanting to provide a schema definition, why also parse the schema definition in this new function?

Is the intent that you would use the response of it to call new GraphQLSchema()?
It doesn't sound like it from the PR description.

@leebyron In PR description I only described the main problem we try to address with this PR. At the same time, sometimes we need to inject type or directive into schema build from user-supplied IDL. Currently, we use string concatenation and concatAST but it addresses only simple scenarios where we injecting static definition for which we already have IDL. In our new library, we want to analyze existing type and have the ability to inject types generated based on this analyzes.

I tried to address both problems in a single function but I'm happy to split it into buildASTDefinitions and buildASTSchemaComponents if needed.

Also, this is also including the standard built-in types in the return, is that what you need from this function? It's a bit strange that built-in types are added but built-in directives are not.

No, it doesn't include standard built-in types. Please see this test-case: https://github.com/graphql/graphql-js/pull/817/files#diff-59b5c9e589eeac323b0666278abb95e9R47
Build types are used only to fill type properties of fields and arguments.

mjmahone · 2018-06-01T01:14:28Z

src/utilities/buildASTSchema.js

@@ -127,6 +130,43 @@ function getNamedTypeNode(typeNode: TypeNode): NamedTypeNode {
 * has no resolve methods, so execution will use default resolvers.
 */
 export function buildASTSchema(ast: DocumentNode): GraphQLSchema {
+  const config: any = buildASTSchemaComponents(ast);


I believe you can fix the any check with:

const config = buildASTSchemaComponents(ast); if (!config.query) { throw new GraphQLError( 'Must provide schema definition with query type or a type named Query.', ast, ); } ... return new GraphQLSchema(config);

I believe the issue for flow is that config.query can be undefined in addition to null.

IvanGoncharov · 2021-08-04T14:15:19Z

Closing since now we can create schemas without root type.

facebook-github-bot added the CLA Signed label Apr 19, 2017

wincent suggested changes Apr 21, 2017

View reviewed changes

IvanGoncharov force-pushed the buildASTSchemaConfig branch 2 times, most recently from 97fa9f0 to 5eb11f0 Compare April 28, 2017 17:01

IvanGoncharov changed the title ~~Add buildASTSchemaConfig function~~ Add buildASTSchemaComponents function Apr 28, 2017

OlegIlyenko mentioned this pull request Apr 29, 2017

Provide convenient functions for IDL-based schema materialization and validation sangria-graphql/sangria#240

Closed

leebyron requested changes May 15, 2017

View reviewed changes

IvanGoncharov force-pushed the buildASTSchemaConfig branch from 5eb11f0 to ad9c2aa Compare May 16, 2017 10:00

leebyron force-pushed the master branch from fda2afd to da8fe62 Compare May 26, 2017 16:58

IvanGoncharov force-pushed the buildASTSchemaConfig branch from ad9c2aa to 089ee42 Compare June 21, 2017 18:43

Add buildASTSchemaComponents function

2ad7595

IvanGoncharov force-pushed the buildASTSchemaConfig branch from 089ee42 to 2ad7595 Compare July 6, 2017 11:05

IvanGoncharov mentioned this pull request Dec 7, 2017

Validate schema root types and directives #1124

Merged

mjmahone reviewed Jun 1, 2018

View reviewed changes

IvanGoncharov force-pushed the master branch from e7509a6 to 16f5ca0 Compare June 26, 2019 17:05

Base automatically changed from master to main January 27, 2021 11:10

IvanGoncharov closed this Aug 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add buildASTSchemaComponents function #817

Add buildASTSchemaComponents function #817

IvanGoncharov commented Apr 19, 2017

wincent left a comment

wincent Apr 21, 2017

IvanGoncharov Apr 22, 2017

wincent Apr 24, 2017

IvanGoncharov Apr 28, 2017

wincent Apr 21, 2017

IvanGoncharov Apr 22, 2017

IvanGoncharov Apr 30, 2017 •

edited

Loading

IvanGoncharov May 15, 2017

wincent Apr 21, 2017

IvanGoncharov Apr 22, 2017

leebyron left a comment

leebyron May 15, 2017

IvanGoncharov May 16, 2017

leebyron May 15, 2017

IvanGoncharov May 16, 2017 •

edited

Loading

IvanGoncharov commented May 16, 2017 •

edited

Loading

mjmahone Jun 1, 2018

IvanGoncharov commented Aug 4, 2021

Add buildASTSchemaComponents function #817

Add buildASTSchemaComponents function #817

Conversation

IvanGoncharov commented Apr 19, 2017

wincent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanGoncharov Apr 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leebyron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanGoncharov May 16, 2017 • edited Loading

Choose a reason for hiding this comment

IvanGoncharov commented May 16, 2017 • edited Loading

Choose a reason for hiding this comment

IvanGoncharov commented Aug 4, 2021

IvanGoncharov Apr 30, 2017 •

edited

Loading

IvanGoncharov May 16, 2017 •

edited

Loading

IvanGoncharov commented May 16, 2017 •

edited

Loading