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

Support merging of schema directives #1003

Merged
merged 5 commits into from
Mar 26, 2019
Merged

Conversation

freiksenet
Copy link
Contributor

@freiksenet freiksenet commented Nov 16, 2018

Add support to merging directive definitions in mergeSchemas.

TODO:

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

@freiksenet freiksenet added the feature New addition or enhancement to existing solutions label Nov 16, 2018
@Torsten85
Copy link

I'm looking forward to this feature and tried your PR, but I get the following error:

const schema = makeExecutableSchema({
  typeDefs: `

    directive @auth(role: Role = ADMIN) on OBJECT | FIELD_DEFINITION

    enum Role {
      ADMIN
      USER
      PUBLIC
    }

    type User {
      firstname: String @auth(role: PUBLIC)
      lastname: String
    }

    type Query {
      user: User
    }
  `
});

const merged = mergeSchemas({ schemas: [schema], mergeDirectives: true });
Schema must contain unique named types but contains multiple types named "Role".

@freiksenet
Copy link
Contributor Author

@Torsten85 thanks a lot, I'll check it and add fix+test for it.

@freiksenet
Copy link
Contributor Author

@Torsten85 Fixed, is it working for you?

Copy link
Contributor

@mfix22 mfix22 left a comment

Choose a reason for hiding this comment

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

LGTM 👍@hwillson should confirm too though

@somil-nagarro
Copy link

somil-nagarro commented Jan 30, 2019

I too waiting for this PR to get merge, please let me know ETA

@andrew-makarenko
Copy link

@benjamn @hwillson could you please take a look?
It's a blocker for me as well. Thank you!

Copy link
Contributor

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Thanks very much for working on this @freiksenet!

@hwillson hwillson merged commit 305466b into master Mar 26, 2019
@hwillson hwillson deleted the schema-directive-definitions branch March 26, 2019 19:40
yaacovCR pushed a commit to yaacovCR/graphql-tools-fork that referenced this pull request Jun 2, 2019
* Support merging of schema directives

* Update CHANGELOG.md

* Add missing branch

* Fix recreation of directives
yaacovCR pushed a commit to yaacovCR/graphql-tools-fork that referenced this pull request Jun 2, 2019
* Support merging of schema directives

* Update CHANGELOG.md

* Add missing branch

* Fix recreation of directives
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New addition or enhancement to existing solutions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants