Add inheritResolversFromInterfaces option #720

reconbot · 2018-04-06T20:10:21Z

This adds inheritResolversFromInterfaces to addResolveFunctionsToSchema and makeExecutableSchema

Later on we'll want to support interfaces implementing other interfaces
graphql/graphql-spec#295

TODO:

If this PR is a new feature, reference an issue where a consensus about the design was reached Types should inherit from interfaces when applying resolver map #616
Make sure all of the significant new logic is covered by tests
Rebase your changes on master so that they can be merged easily
Make sure all tests and linter rules pass
Update CHANGELOG.md with your change. Include a description of your change, link to PR (always) and issue (if applicable). Add your CHANGELOG entry under vNEXT. Do not create a new version number for your change yourself.

reconbot · 2018-04-09T17:07:23Z

cc @benjamn =)

benjamn

Looking good so far! Just a few suggestions regarding options parameter style.

benjamn · 2018-04-10T13:38:51Z

src/schemaGenerator.ts

  resolverValidationOptions: IResolverValidationOptions = {},
+  inheritResolversFromInterfaces: boolean = false


Rather than adding another boolean positional argument, let's merge the validation options and the resolver inheritance option into a single options parameter. This will probably require a more general name for the IResolverValidationOptions type, such as IAddResolveFunctionsToSchemaOptions.

More on why boolean positional arguments are problematic: https://ariya.io/2011/08/hall-of-api-shame-boolean-trap

I completely agree but I was avoiding making a breaking change to the public api. I'll see about supporting call signatures with a warning as discussed on twitter.

benjamn · 2018-04-10T13:39:56Z

src/schemaGenerator.ts

@@ -69,6 +69,7 @@ function _generateSchema(
  allowUndefinedInResolve: boolean,
  resolverValidationOptions: IResolverValidationOptions,
  parseOptions: GraphQLParseOptions,
+  inheritResolversFromInterfaces: boolean


This boolean argument is fine because _generateSchema is private to this module (and not exported like addResolveFunctionsToSchema).

benjamn · 2018-04-10T13:42:04Z

src/schemaGenerator.ts

+  const typeNames = new Set([
+    ...Object.keys(schema.getTypeMap()),
+    ...Object.keys(resolvers)
+  ]);


A little simpler?

const typeNames = Object.keys({ ...schema.getTypeMap(), ...resolvers, });

Having an array rather than a Set should be fine because the only usage below is calling forEach.

benjamn · 2018-04-10T13:45:15Z

src/schemaGenerator.ts

+      return;
+    }
+
+    const interfaceResolvers = (type as GraphQLObjectType).getInterfaces().map((iFace) => resolvers[iFace.name]);


You might be able to reorganize this code so that the type.getInterfaces() call is inside a type instanceof GraphQLObject conditional block, so you don't have to explicitly cast it. Then you could put the code above in the else branch and avoid the early return, too.

But I love returning early!

But I also see what you mean and this is a much cleaner suggestion.

reconbot

updated with a new interface

reconbot · 2018-04-10T13:53:29Z

src/schemaGenerator.ts

  resolverValidationOptions: IResolverValidationOptions = {},
+  inheritResolversFromInterfaces: boolean = false


I completely agree but I was avoiding making a breaking change to the public api. I'll see about supporting call signatures with a warning as discussed on twitter.

reconbot · 2018-04-10T13:57:31Z

src/schemaGenerator.ts

+      return;
+    }
+
+    const interfaceResolvers = (type as GraphQLObjectType).getInterfaces().map((iFace) => resolvers[iFace.name]);


But I love returning early!

But I also see what you mean and this is a much cleaner suggestion.

reconbot · 2018-04-10T14:20:40Z

(working on docs but welcome a review on the changes)

reconbot · 2018-04-10T15:56:16Z

Added some docs about the new api and rebased

This adds `inheritResolversFromInterfaces` to `addResolveFunctionsToSchema` and `makeExecutableSchema` Later on we'll want to support interfaces implementing other interfaces graphql/graphql-spec#295

…s pr concerns

benjamn · 2018-04-10T18:48:45Z

src/schemaGenerator.ts

+      schema: options,
+      resolvers: legacyInputResolvers,
+      resolverValidationOptions: legacyInputValidationOptions
+    };


reconbot · 2018-04-10T18:55:03Z

🎉

ghost added the feature New addition or enhancement to existing solutions label Apr 6, 2018

reconbot force-pushed the interface-resolver-inheritance branch from bf9e1af to 3e01db4 Compare April 6, 2018 20:17

ghost added the feature New addition or enhancement to existing solutions label Apr 6, 2018

reconbot force-pushed the interface-resolver-inheritance branch from 3e01db4 to 0b1931f Compare April 6, 2018 20:20

reconbot changed the title ~~Add inheritResolversFromInterfaces option~~ Add inheritResolversFromInterfaces option Apr 6, 2018

ghost added the feature New addition or enhancement to existing solutions label Apr 6, 2018

benjamn suggested changes Apr 10, 2018

View reviewed changes

reconbot commented Apr 10, 2018

View reviewed changes

reconbot added 3 commits April 10, 2018 11:57

Add inheritResolversFromInterfaces option

4fea9ac

This adds `inheritResolversFromInterfaces` to `addResolveFunctionsToSchema` and `makeExecutableSchema` Later on we'll want to support interfaces implementing other interfaces graphql/graphql-spec#295

Add a new options object for addResolveFunctionsToSchema and addres…

06f25c2

…s pr concerns

docs

302e1a6

reconbot force-pushed the interface-resolver-inheritance branch from 8906bed to 302e1a6 Compare April 10, 2018 15:57

benjamn approved these changes Apr 10, 2018

View reviewed changes

Merge branch 'master' into interface-resolver-inheritance

af91e11

benjamn merged commit c470ffc into ardatan:master Apr 10, 2018

reconbot deleted the interface-resolver-inheritance branch April 10, 2018 18:53

This was referenced Apr 11, 2018

Update graphql-tools in / from 2.10.0 to 2.24.0 LibreTubeApp/LibreTube#55

Open

Update graphql-tools in /modules/api from 2.23.1 to 2.24.0 project-accelerate/accelerate#118

Open

Druotic mentioned this pull request May 29, 2018

Add inheritResolversFromInterfaces option - mergeSchemas #812

Merged

5 tasks

MichalLytek mentioned this pull request Feb 23, 2019

[Feature request] Resolvers for interfaces MichalLytek/type-graphql#260

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add inheritResolversFromInterfaces option #720

Add inheritResolversFromInterfaces option #720

reconbot commented Apr 6, 2018 •

edited

Loading

reconbot commented Apr 9, 2018

benjamn left a comment

benjamn Apr 10, 2018

reconbot Apr 10, 2018

benjamn Apr 10, 2018

benjamn Apr 10, 2018

benjamn Apr 10, 2018

reconbot Apr 10, 2018

reconbot left a comment

reconbot Apr 10, 2018

reconbot Apr 10, 2018

reconbot commented Apr 10, 2018 •

edited

Loading

reconbot commented Apr 10, 2018 •

edited

Loading

benjamn Apr 10, 2018

reconbot commented Apr 10, 2018

		resolverValidationOptions: IResolverValidationOptions = {},
		inheritResolversFromInterfaces: boolean = false

Add inheritResolversFromInterfaces option #720

Add inheritResolversFromInterfaces option #720

Conversation

reconbot commented Apr 6, 2018 • edited Loading

reconbot commented Apr 9, 2018

benjamn 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

reconbot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reconbot commented Apr 10, 2018 • edited Loading

reconbot commented Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

reconbot commented Apr 10, 2018

reconbot commented Apr 6, 2018 •

edited

Loading

reconbot commented Apr 10, 2018 •

edited

Loading

reconbot commented Apr 10, 2018 •

edited

Loading