Skip to content

Add support for custom enum resolvers and scalar types#1345

Merged
trevor-scheer merged 7 commits intoapollographql:masterfrom
hannesj:graphql-tools-resolvers-to-schema
Jun 25, 2019
Merged

Add support for custom enum resolvers and scalar types#1345
trevor-scheer merged 7 commits intoapollographql:masterfrom
hannesj:graphql-tools-resolvers-to-schema

Conversation

@hannesj
Copy link
Copy Markdown
Contributor

@hannesj hannesj commented Jun 13, 2019

As described in #1343 and #1344, it is better to use addResolveFunctionsToSchema from graphql-tools to add the resolvers to the new schema, as it is how the resolvers are added normally in apolo-server, so that both scalars and enums are handled properly. As a TODO, it might be sensible to use more functionality from https://github.com/apollographql/graphql-tools/blob/84e77ac7d7e06f149019c5a42dd2e86907e27ac1/src/makeExecutableSchema.ts when generating the new schema.

TODO:

  • Update CHANGELOG.md* with your change (include reference to issue & this PR)
  • 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

*Make sure changelog entries note which project(s) has been affected. See older entries for examples on what this looks like.

@JacksonKearl
Copy link
Copy Markdown

Thanks! This feels much better. Ping @trevor-scheer to ask if this is a bad idea for any reason.

@trevor-scheer
Copy link
Copy Markdown
Contributor

Hey @hannesj, firstly thank you for the details and the contribution. Your analysis and approach are perfectly reasonable.

graphql-tools was intentionally left out as a dependency in this project as it's something that we're looking to move away from going forward (Apollo Server included, at some point). I do think that we want to duplicate the missing logic to this function for now (re: abstract and enum types).

Apologies that this isn't documented or explained anywhere yet, but I'd like to add some additional context so this isn't coming out of nowhere.

GraphQL tools includes two major things:

  • A way to build a schema from SDL + resolvers
  • Schema stitching

We're currently working with maintainers of graphql-js to move most of the tools we wrote to build schemas into the core.

With Federation, we're moving away from stitching. Since many teams still rely on stitching, we want to make the graphql-tools package a community-owned package and are working on a transition plan for it.

@hannesj hannesj force-pushed the graphql-tools-resolvers-to-schema branch from e5ce9ea to 4fa08ae Compare June 17, 2019 10:25
@hannesj
Copy link
Copy Markdown
Contributor Author

hannesj commented Jun 17, 2019

Hi,

I rebased this based on feedback from you, leaving out the adding of the dependency, and using the previous commits.

Unfortunately it seems that the solution for renaming the enums was not enough, as the lastly added test now fails.


// In place updating hack to get around pulling in the full
// schema walking and immutable updating machinery from graphql-tools
Object.assign(
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor

@trevor-scheer trevor-scheer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, I'd like @jbaxleyiii to give approval on the approach in case there's any nuance I'm overlooking.

Comment thread packages/apollo-graphql/src/schema/buildSchemaFromSDL.ts Outdated
Comment thread packages/apollo-graphql/src/schema/buildSchemaFromSDL.ts Outdated
@JacksonKearl JacksonKearl requested a review from jbaxleyiii June 19, 2019 17:30
@JacksonKearl JacksonKearl changed the title Use addResolveFunctionsToSchema from graphql-tools Add support for custom enum resolvers and scalar types Jun 24, 2019
@trevor-scheer trevor-scheer merged commit 933c3d4 into apollographql:master Jun 25, 2019
@trevor-scheer
Copy link
Copy Markdown
Contributor

Thank you both for this @hannesj and @JacksonKearl! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants