Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add directiveResolvers param to mergeSchemas #579

Closed
wants to merge 3 commits into from
Closed

Add directiveResolvers param to mergeSchemas #579

wants to merge 3 commits into from

Conversation

mfix22
Copy link
Contributor

@mfix22 mfix22 commented Jan 12, 2018

Hey guys,
This PR adds the ability to wrap a merged schema with directive resolvers. This is beneficial say, when you want to wrap your entire merged schema with a common @auth directive.

mergeSchemas({ schemas, resolvers, directiveResolvers })

However, this currently doesn't work with remote schemas (because directive information is lost when we printSchema graphql/graphql-js#869), although I am trying to get that working to. Thought I would get some feedback first before going through with the whole thing 😄

TODO:

  • Update
  • If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
  • 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

@freiksenet
Copy link
Contributor

The AST being lost should be resolved now. I'm wary of adding of new stuff to mergeSchemas atm cause we are heavily rethinking the api. I think atm you can just call attachDirectiveResolvers directly on resulting mergedSchema if you want to add them.

@mfix22
Copy link
Contributor Author

mfix22 commented Feb 5, 2018

@freiksenet that sounds like a good alternative. I wrote this before seeing the Next-API PR. I'll go ahead and close and I can revisit this after the new API lands if anyone wants it.

@mfix22 mfix22 closed this Feb 5, 2018
@mfix22 mfix22 deleted the directiveResolversInMergeSchemas branch February 5, 2018 16:42
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.

2 participants