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

Remove all 'instanceof GraphQLSchema' checks #371

Merged
merged 1 commit into from
May 5, 2016
Merged

Remove all 'instanceof GraphQLSchema' checks #371

merged 1 commit into from
May 5, 2016

Conversation

helfer
Copy link
Contributor

@helfer helfer commented Apr 28, 2016

This change removes the instanceof GraphQLSchema checks in validate and execute. This makes graphql-js play more nicely with schemas generated by other libraries, or even schemas created by other versions of graphql-js as long as they are compatible (a common issue with npm v2).

@leebyron Right now I just removed the checks and didn't replace them with anything, because it turned out that no tests were affected by the change. Let me know if you think we should have some duck typing checks in there, such as checking that getTypeMap, get Type, getQueryType, getDirectives etc. are functions.

@ghost ghost added the CLA Signed label Apr 28, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0008%) to 98.249% when pulling f4fe2f7 on apollostack:drop-instanceof into ec05b54 on graphql:master.

@leebyron leebyron merged commit 71df461 into graphql:master May 5, 2016
@leebyron
Copy link
Contributor

leebyron commented May 5, 2016

Yeah, those invariants were probably a little over-protective. I think this is fine.

@leebyron
Copy link
Contributor

leebyron commented May 5, 2016

See the revert - this actually caused problems :(

sogko added a commit to sogko/graphql-js that referenced this pull request Jun 1, 2016
* master: (26 commits)
  0.6.0
  Validation: improving overlapping fields quality (graphql#386)
  Validation: context.getFragmentSpreads now accepts selectionSet rather than fragment AST node
  Factor out more closure functions
  Factor out closure functions to normal functions
  Deprecated directive (graphql#384)
  RFC: Directive location: schema definition (graphql#382)
  RFC: Schema Language Directives (graphql#376)
  Export introspection in public API
  Export directive definitions. (graphql#381)
  BUG: Ensure building AST schema does not exclude @Skip and @include (graphql#380)
  documentation of schema constructor
  Revert "Remove all 'instanceof GraphQLSchema' checks" (graphql#377)
  remove all 'instanceof GraphQLSchema' checks (graphql#371)
  Error logging for new interface type semantics (graphql#350)
  Nit: Missing case in grammar for TypeSystemDefinition in comment
  Bug: printer can print non-parsable value
  Factor out suggestion quoting utility
  Minor refactoring
  Minor refactoring of error messages for unknown fields
  ...
@rdrey
Copy link

rdrey commented Aug 27, 2016

@helfer I just ran into issues with this when I npm linked graphql-tools. I unlinked it and am running OK again. (I don't have any changes to graphql-tools, just linked it to debug something, so I don't need it linked permanently.) Just thought I'd mention that here.

Would linking the same graphql into both my repo and graphql-tools be a possible workaround?

@helfer
Copy link
Contributor Author

helfer commented Aug 27, 2016

@rdrey yes, that should do the trick. But to be honest, sometimes I found it to be faster to use npm publish under a fake package name than to manually fix all the dependencies that npm link doesn't manage to get right.

@helfer
Copy link
Contributor Author

helfer commented Aug 27, 2016

Also, FWIW: I would now solve the problem that this PR was supposed to help with by extending the base classes from graphql-js in my own package, if I needed to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants