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

Added extend support to schemaGenerator #90

Merged
merged 3 commits into from
Aug 5, 2016

Conversation

davidyaha
Copy link
Contributor

@davidyaha davidyaha commented Aug 4, 2016

This PR is meant to allow use of the following IDL syntax:
https://github.com/graphql/graphql-js/blob/9ea8196c2af97551bab5cfd57201c29e3085ee4e/src/utilities/__tests__/extendSchema.js#L127

It seems to a feature that was meant to be used for client purposes only.
But, in my opinion, while using the IDL to describe type definitions, you lose to ability to do cyclic dependencies and by that, the ability to create suite packages that will enhance and be enhanced by your own application code base.

e.g:

Consider the accounts package as the sole owner of the User type, being so, in your own schema files, you would like to use that same User type to describe relations to other types.
Lets say Post

type Post {
  id: ID!
  creator: User
}

And so you would also like to expand on the User type to allow for relations to the Post type:

extend type User {
  posts: [Post]
}

Other possible solutions could be to use union types but these will not allow to get the extended User type on the queries provided by the accounts package

This PR is not harming any previous features of the schemaGenerator.
One current limitation is that after you've added an extend type definition, you have to provide type resolvers for all fields.

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

@apollo-cla
Copy link

@davidyaha: 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/

@@ -1,5 +1,8 @@
# Changelog

### v0.6.2
Copy link
Contributor

Choose a reason for hiding this comment

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

rename this to vNEXT (we pick the version when we publish)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do! Wasn't sure what to put here 😅

@helfer
Copy link
Contributor

helfer commented Aug 4, 2016

@davidyaha Thanks a lot, this looks pretty good! Can you add one test that checks that extensions also work when you define them in different files? To test that, you can just have an array with individual definitions (type strings) as elements instead of having just one long string. That will simulate importing from different files.

@davidyaha
Copy link
Contributor Author

Hey @helfer, thanks for the review. The first test I've added uses an array of type definitions. Would you like me to add another one with a string for the more common case?

@helfer
Copy link
Contributor

helfer commented Aug 4, 2016

You're right, in that case it's fine as it is!

The only remaining thing that would be nice to have is if there was a test for this line:
https://github.com/apollostack/graphql-tools/pull/90/files#diff-808ec356ff8ddeab265abfaefebaaf00R138

Right now, that doesn't seem to be covered. If there's no way that line can be reached anyway, then you can remove the whole test altogether.

… throwing an error or creating an AST document.

* Also changed version on changelog to vNEXT as required
@davidyaha
Copy link
Contributor Author

Absolutely right. There is no need for that check.

@helfer
Copy link
Contributor

helfer commented Aug 5, 2016

Great, thanks for the PR!

@helfer helfer merged commit 145801c into ardatan:fix-build-dist Aug 5, 2016
@helfer
Copy link
Contributor

helfer commented Aug 5, 2016

@davidyaha Oh, hey, can you make this PR against master? I think you made it against an already merged branch by accident.

@davidyaha davidyaha mentioned this pull request Aug 5, 2016
4 tasks
@stubailo
Copy link
Contributor

Holy crap, we have this feature?!?!!? why is it not documented anywhere!

@davidyaha
Copy link
Contributor Author

Hehe, my first PR here.. been telling people about it on slack when ever the subject poped.. but didn't think of adding it to the docs.
Main reason is that it is kind of no intended to be used for servers... Itwas actually throwing an error on purpose if you did not specify a resolver for every field your querying. I think with the current version of GraphQL.js it's only throwing for interfaces and unions..
Been using js string templates instead for a while..

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