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 option to pass schema as string, fixes #59 #78

Merged
merged 2 commits into from
Jun 30, 2017

Conversation

christophercliff
Copy link
Contributor

@christophercliff christophercliff commented Jun 27, 2017

Attempted fix for #59. Could you please let me know if this is looks appropriate and what's expected in terms of test?

TODO:

  • 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 pass
  • Update CHANGELOG.md with your change
  • If this was a change that affects the external API, update the README

@apollo-cla
Copy link

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

@jnwng
Copy link
Contributor

jnwng commented Jun 28, 2017

@christophercliff beautiful, i was just working on this too! glad you got it all working. left a couple of comments, and do you mind filling out the Meteor CLA?

test/makeRule.js Outdated
@@ -26,7 +28,7 @@ const parserOptions = {

{
const options = [
{ schemaJson },
{ schemaString },
Copy link
Contributor

Choose a reason for hiding this comment

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

i dont think you want to replace this — you should add a separate block of tests for this

@@ -53,6 +54,9 @@ const defaultRuleProperties = {
schemaJsonFilepath: {
type: 'string',
},
schemaString: {
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think about using typeDefs instead? its a term i saw a lot in graphql-tools when referring to the schema when defined via the IDL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makeExecutableSchema() defines typeDefs:

typeDefs is a required argument and should be an array of GraphQL schema language strings or a function that takes no arguments and returns an array of GraphQL schema language strings. The order of the strings in the array is not important, but it must include a schema definition.

That's not exactly what's used here. Alternatively, buildSchema() just refers to it as source. printSchema() refers to it as "schema in the Schema Language format".

I think schemaString is a decent compromise given the ambiguity and that the API is already using schemaJson.

@jnwng
Copy link
Contributor

jnwng commented Jun 28, 2017

also, do you mind updated the README accordingly?

@christophercliff
Copy link
Contributor Author

@jnwng Please see updates. Could you give me an idea when this might be published?

@jnwng jnwng merged commit 133219c into apollographql:master Jun 30, 2017
@jnwng
Copy link
Contributor

jnwng commented Jun 30, 2017

thank you @christophercliff! this should be available in [email protected] (which is live)

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