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

pass directives from schema modules through buildServiceDefinition #715

Merged
merged 3 commits into from
Dec 7, 2018

Conversation

tgerk
Copy link
Contributor

@tgerk tgerk commented Nov 16, 2018

Directive definitions were discarded when merging types from modules in buildServiceSchema. The fix is nearly trivial, because Kind.DIRECTIVE_DEFINITION is a unique case that is neither a schema definition nor a type definition.

@apollo-cla
Copy link

@tgerk: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@martijnwalraven
Copy link
Contributor

Thanks for looking into this! I don't think this is the right solution though, because directives live in their own namespace, and adding them to typeDefinitionsMap clashes with similarly named type definitions.

I think the solution is to keep a separate list of directiveDefinitions, similar to what buildASTSchema does (see here).

Also, it seems this PR needs to be rebased, because it includes unrelated commits made by others.

@tgerk
Copy link
Contributor Author

tgerk commented Nov 20, 2018

Pardon the confusion, I have rebased my branch from the tagged release to master branch.

I've also segregated the directives from types as you pointed out.

Copy link
Member

@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.

Aside from my single comment, this LGTM. Would like @martijnwalraven to give final pass and approval.

packages/apollo-tools/src/buildServiceDefinition.ts Outdated Show resolved Hide resolved
@tgerk tgerk changed the title Issue 714 pass directives from modules through buildServiceDefinition Dec 7, 2018
@tgerk tgerk changed the title pass directives from modules through buildServiceDefinition pass directives from schema modules through buildServiceDefinition Dec 7, 2018
@tgerk
Copy link
Contributor Author

tgerk commented Dec 7, 2018

Changes have been made to variable names and to behavior as suggested, including tests.

  • duplicated directive declarations within a module or in different modules are detected and reported
  • directives and type definitions are used together in constructing the resulting schema

@martijnwalraven martijnwalraven merged commit de6a7af into apollographql:master Dec 7, 2018
@martijnwalraven
Copy link
Contributor

Thanks for opening this PR and making the changes! (The CI failures are unrelated I think.)

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.

4 participants