search

matthewmueller / graph.ql

Faster and simpler way to create GraphQL servers
633 stars 34 forks source link

Add type to the schema to fix the type missing issue #23

Open Aleph-Z opened 8 years ago

Aleph-Z commented 8 years ago
matthewmueller commented 8 years ago

thanks for the PR, can you explain what you mean by:

Add types to the schema to fix the type missing issue

And why remove co?

Aleph-Z commented 8 years ago

The former is for issue #22 'Interface implementations missing from schema with graphql-js 0.5.0'. Agree with jimkyndemeyer that adding all_types to the schema should be the default behaviour.

The latter is because co.wrap is not necessary for synchronous functions or functions returning a Promise, and co.wrap can be done outside of graph.ql, in schema definition if needed.

matthewmueller commented 8 years ago

cool, so i'm 👍 to:

• Add types to the schema to fix the type missing issue • Upgrade graphql to ^0.6.0

and :-1: to:

• Remove co

You're right that you can do it outside, but it's a lot more seamless to handle it internally. If it's causing an issue for you or you're concerned about performance, I'm happy to conditionally check if what is passed in is a generator using: https://github.com/sindresorhus/is-generator-fn

• Move graphql to peerDependencies

I've never been a fan of peerDependencies and with NPM@3 they aren't installed automatically so this package would basically come broken for NPM@3 users.

Happy to merge this when those changes are made. Thanks!

Aleph-Z commented 8 years ago

You are right, it's better using is-generator-fn.

About the peerDependencies, I got a error "Schema must be an instance of GraphQLSchema. Also ensure that there are not multiple versions of GraphQL installed in your node_modules directory." during the execution after I installed graph.ql the first time, since I had another version of graphql installed before, if graphql was defined in peerDependencies in graph.ql, NPM would be able to report the error on the first place. Removing graphql before adding graph.ql can avoid this error too.

Move graphql back to dependencies, and add comment to the Installation part of Readme.md.