Add 'toConfig' methods to all type system definitions objects #1331

IvanGoncharov · 2018-05-01T17:16:55Z

I created this PR to get early feedback about toConfig feature so it's missing proper tests.

toConfig is intended to help with schema transformation by minimizing the amount of code you need write and maintain, e.g. recreateType from graphql-tools.
When I worked on graphql-voyager I created my own intermidiate format just to do schema transformations.

Moreover, toConfig will prevent bugs that occur when a new field is added. For example, lexicographicSortSchema doesn't respect newly added assumeValid and allowedLegacyNames fields.

IvanGoncharov · 2018-05-01T17:28:22Z

src/type/definition.js

+  description: ?string,
+  astNode?: ?ObjectTypeDefinitionNode,
+  extensionASTNodes: $ReadOnlyArray<ObjectTypeExtensionNode>,
+|};


I can't use GraphQL*Config types since they have Thunk fields and a lot of fields marked as optional. Because without this new type all code that uses toConfig should have a lot of unnecessary runtime checks and also have dead code to handle Thunk.

Plus I think it easier to keep this types up to date than update extendSchema, lexicographicSortSchema, and all similar functions inside 3rd-party tools.

I wonder whether we still need the Thunk fields in the GraphQL*Config types at all. This feels like a much cleaner expression of a GraphQL type without needing surrounding schema. I'll admit, I don't fully understand how the thunks are used currently, so take that idea as a relative newcomer's curiosity instead of a strong suggestion.

As far as I have always understood, the thunks are there to avoid circular references at module import time when your schema is made up of types each in their own module. We have definitely run into that situation a couple of times and switched to using thunks.

For toConfig, however, it’s indeed not necessary as by the time it can be invoked the import stage should already have passed.

freiksenet

This is amazing and super useful! Let's get it shipped! 👍 💯

Tests are needed here I guess.

freiksenet · 2018-05-31T07:33:14Z

I've checked it against recreateType in graphql-tools and it looks like it matches what we have there, so it should be fine.

mjmahone

This is great! I have (internal to my repo) a few tools that are transforming the regular schema into an introspection schema and back, in order to do things like type trimming. Having this available would have made that code significantly cleaner!

I do want to make sure this doesn't end up in the public API in 14.0 though (I see you haven't modified any index.js, which is great): I still am not totally sure about the internal representation being best optimized for schema modification, and I think as a community we need to play around with it a bit to be sure. It would be annoying to have to cut a new major release just because we changed the GraphQL*ExactConfig API to get performance improvements, for example.

mjmahone · 2018-06-02T19:03:38Z

src/type/definition.js

+  description: ?string,
+  astNode?: ?ObjectTypeDefinitionNode,
+  extensionASTNodes: $ReadOnlyArray<ObjectTypeExtensionNode>,
+|};


I wonder whether we still need the Thunk fields in the GraphQL*Config types at all. This feels like a much cleaner expression of a GraphQL type without needing surrounding schema. I'll admit, I don't fully understand how the thunks are used currently, so take that idea as a relative newcomer's curiosity instead of a strong suggestion.

mjmahone · 2018-06-02T19:19:12Z

src/type/definition.js

@@ -782,6 +853,16 @@ export type GraphQLObjectTypeConfig<TSource, TContext> = {
  extensionASTNodes?: ?$ReadOnlyArray<ObjectTypeExtensionNode>,
 };

+export type GraphQLObjectTypeExactConfig<TSource, TContext> = {|
+  name: string,
+  interfaces: Array<GraphQLInterfaceType>,


Should this instead be:

interfaces: Array<GraphQLInterfaceTypeExactConfig<TSource, TContext>>,

I'm not sure which makes sense: we definitely don't want to end up with a Schema where an object points to a different instance of the same interface vs. what the schema itself has. So we need to make sure, anywhere we hydrate a type out of this config, that it ends up pointing to the schema's actual interface type, rather than the interface type used at build time (I see you're already doing this everywhere it's used, but others might not be so prudent). I'd even be tempted to make this just Array<string> just to prevent any mismatches. At that point, we're basically be creating a stricter version of the Introspection schema (we seem to need a non-Introspection, but Introspection-like, representation regardless, so I'm still OK with this).

My worry may be a complete non-issue if we don't allow exporting these new types and limit how we use them within the repo.

alloy · 2018-10-25T09:54:40Z

@IvanGoncharov I’d love to have access to this 🙏 Currently I’ve copy-pasted some code from the extendSchema module into my own code https://github.com/artsy/metaphysics/pull/1346/files#diff-2092cb52bba31c5ad29eb09c6aa4c475R57.

I agree with the idea of not exporting this through the top-level index.js file yet, but other than that is there anything still blocking this?

mjmahone · 2019-01-16T15:41:02Z

src/type/definition.js

@@ -674,6 +696,23 @@ export class GraphQLObjectType {
    return this._interfaces;
  }

+  toConfig(): {|
+    ...GraphQLObjectTypeConfig<*, *>,


There's definitely another project for someone of figuring out how to get rid of all these * flow types (which are about as bad as any). Not this PR though.

mjmahone

This looks very good to me. It would be good to test the new toConfig function, but given we're already using it in other functions that are already well-tested, I'm not very concerned about it being wrong.

mjmahone · 2019-01-16T15:42:24Z

src/type/schema.js

@@ -238,6 +238,27 @@ export class GraphQLSchema {
  getDirective(name: string): ?GraphQLDirective {
    return find(this.getDirectives(), directive => directive.name === name);
  }
+
+  toConfig(): {|
+    ...GraphQLSchemaConfig,


huh this must be new Flow wizardry: this is a nicely concise way of defining types in terms of other types!

langpavel

Nice work!

See graphql/graphql-js#1331

* Add Martijn Walraven to "Definitions by" section of the package header * Add `validateSDL` function * Add `toConfig` methods See graphql/graphql-js#1331 * Add missing typing for `print` function See graphql/graphql-js#1702 * Rename `MaybePromise` to `PromiseOrValue` graphql/graphql-js#1798 * Rename `blockStringValue.js` to `blockString.js`, and `blockStringValue` to `dedentBlockStringValue` See graphql/graphql-js@16afd2e and graphql/graphql-js@60a2bf8 * Update `graphql` version to 14.2 * Add `.prettierrc` and reformat all files This adds a `.prettierrc` based on the defaults used in #24552 (comment), and reformats all files (only a few are affected). * Fix `GraphQLSchema.toConfig()` * Remove `.prettierrc`

* Add Martijn Walraven to "Definitions by" section of the package header * Add `validateSDL` function * Add `toConfig` methods See graphql/graphql-js#1331 * Add missing typing for `print` function See graphql/graphql-js#1702 * Rename `MaybePromise` to `PromiseOrValue` graphql/graphql-js#1798 * Rename `blockStringValue.js` to `blockString.js`, and `blockStringValue` to `dedentBlockStringValue` See graphql/graphql-js@16afd2e and graphql/graphql-js@60a2bf8 * Update `graphql` version to 14.2 * Add `.prettierrc` and reformat all files This adds a `.prettierrc` based on the defaults used in DefinitelyTyped#24552 (comment), and reformats all files (only a few are affected). * Fix `GraphQLSchema.toConfig()` * Remove `.prettierrc`

facebook-github-bot added the CLA Signed label May 1, 2018

IvanGoncharov requested a review from leebyron May 1, 2018 17:19

IvanGoncharov commented May 1, 2018

View reviewed changes

freiksenet reviewed May 31, 2018

View reviewed changes

IvanGoncharov mentioned this pull request Jun 1, 2018

Use empty arrays instead of undefined properties #1259

Merged

mjmahone approved these changes Jun 2, 2018

View reviewed changes

IvanGoncharov force-pushed the toConfig branch from 6d433a0 to 0dc4c3f Compare June 3, 2018 02:34

IvanGoncharov mentioned this pull request Sep 7, 2018

Upgrade graphql peerDependency to 14.0.0 ardatan/graphql-tools#945

Closed

IvanGoncharov force-pushed the toConfig branch 3 times, most recently from c27a2f4 to b318257 Compare January 10, 2019 11:52

Add 'toConfig' method

c8d1c28

IvanGoncharov force-pushed the toConfig branch from b318257 to c8d1c28 Compare January 10, 2019 16:49

mjmahone reviewed Jan 16, 2019

View reviewed changes

mjmahone approved these changes Jan 16, 2019

View reviewed changes

langpavel approved these changes Jan 16, 2019

View reviewed changes

IvanGoncharov added this to the v14.2.0 milestone Jan 16, 2019

IvanGoncharov added the PR: feature 🚀 requires increase of "minor" version number label Jan 17, 2019

IvanGoncharov merged commit 92e647c into graphql:master Jan 18, 2019

IvanGoncharov changed the title ~~[RFC]Add 'toConfig' method~~ Add 'toConfig' methods Jan 18, 2019

IvanGoncharov deleted the toConfig branch January 20, 2019 02:03

martijnwalraven added a commit to apollographql/DefinitelyTyped that referenced this pull request Mar 6, 2019

Add toConfig methods

bc8de4f

See graphql/graphql-js#1331

IvanGoncharov changed the title ~~Add 'toConfig' methods~~ Add 'toConfig' methods to all type system definitions objects Mar 26, 2019

calebfaruki mentioned this pull request Jun 11, 2019

[Federation] TypeError: schema.toConfig is not a function apollographql/apollo-server#2825

Closed

snyk-bot mentioned this pull request Aug 31, 2019

[Snyk] Upgrade graphql from 14.1.1 to 14.5.0 sandorTuranszky/production-ready-expressjs-server#13

Merged

snyk-bot mentioned this pull request Jul 2, 2021

[Snyk] Upgrade cspell from 5.2.4 to 5.6.2 turkdevops/graphql-js#237

Closed

snyk-bot mentioned this pull request Jul 3, 2021

[Snyk] Upgrade cspell from 5.2.4 to 5.6.3 turkdevops/graphql-js#239

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'toConfig' methods to all type system definitions objects #1331

Add 'toConfig' methods to all type system definitions objects #1331

IvanGoncharov commented May 1, 2018 •

edited

IvanGoncharov May 1, 2018 •

edited

mjmahone Jun 2, 2018

alloy Oct 25, 2018

freiksenet left a comment •

edited

freiksenet commented May 31, 2018

mjmahone left a comment

mjmahone Jun 2, 2018

mjmahone Jun 2, 2018

alloy commented Oct 25, 2018

mjmahone Jan 16, 2019

mjmahone left a comment

mjmahone Jan 16, 2019

langpavel left a comment

Navigation Menu

Add 'toConfig' methods to all type system definitions objects #1331

Add 'toConfig' methods to all type system definitions objects #1331

Conversation

IvanGoncharov commented May 1, 2018 • edited

IvanGoncharov May 1, 2018 • edited

Choose a reason for hiding this comment

mjmahone Jun 2, 2018

Choose a reason for hiding this comment

alloy Oct 25, 2018

Choose a reason for hiding this comment

freiksenet left a comment • edited

Choose a reason for hiding this comment

freiksenet commented May 31, 2018

mjmahone left a comment

Choose a reason for hiding this comment

mjmahone Jun 2, 2018

Choose a reason for hiding this comment

mjmahone Jun 2, 2018

Choose a reason for hiding this comment

alloy commented Oct 25, 2018

mjmahone Jan 16, 2019

Choose a reason for hiding this comment

mjmahone left a comment

Choose a reason for hiding this comment

mjmahone Jan 16, 2019

Choose a reason for hiding this comment

langpavel left a comment

Choose a reason for hiding this comment

IvanGoncharov commented May 1, 2018 •

edited

IvanGoncharov May 1, 2018 •

edited

freiksenet left a comment •

edited