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

fix bugs in EnumValue #100

Merged
merged 1 commit into from
Jan 15, 2018
Merged

Conversation

NeoPhi
Copy link
Contributor

@NeoPhi NeoPhi commented Jan 8, 2018

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

@jnwng
Copy link
Contributor

jnwng commented Jan 10, 2018

thanks @NeoPhi, just confirming that this is the best way to get this information for enums. im looking into the upstream issues with graphql-import that are showing up in travis

@NeoPhi
Copy link
Contributor Author

NeoPhi commented Jan 10, 2018

Based on how https://github.com/graphql/graphql-js/blob/master/src/utilities/findDeprecatedUsages.js#L49 is implemented, I've not found another way to get access to getEnumValue given that https://github.com/graphql/graphql-js/blob/master/src/validation/validate.js#L97 doesn't expose that utility function nor does it export typeInfo.

@jnwng
Copy link
Contributor

jnwng commented Jan 10, 2018

oh sorry @NeoPhi i had meant that i was confirming that "yes, this is the best way to do this". im figuring out what to do about upstream dependencies that don't match our engine support before merging / releasing this.

@jnwng jnwng merged commit c1e2dd6 into apollographql:master Jan 15, 2018
@jnwng
Copy link
Contributor

jnwng commented Jan 15, 2018

finally — figured out what was going on in an upstream dependency that was being triggered here. turns out the Travis test runner uses npm instead of yarn, but in this case that was actually good as it revealed that we had dependencies that didn't work properly in Node v4/v5.

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.

2 participants