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 result in favor of the key's returned by the query #31

Merged
merged 4 commits into from Apr 26, 2016

Conversation

Projects
None yet
4 participants
@jbaxleyiii
Copy link
Member

jbaxleyiii commented Apr 24, 2016

Discussion in #30

cc @abhiaiyer91

James Baxley
@jbaxleyiii

This comment has been minimized.

Copy link
Member Author

jbaxleyiii commented Apr 24, 2016

I'm going to give this a day for people to chime in if they are interested / opposed to this change.

cc @stubailo @apollostack/community-contributors @johnthepink

@johnthepink

This comment has been minimized.

Copy link
Contributor

johnthepink commented Apr 24, 2016

👍

@abhiaiyer91

This comment has been minimized.

Copy link
Contributor

abhiaiyer91 commented Apr 24, 2016

I'm in :)

@abhiaiyer91

This comment has been minimized.

Copy link
Contributor

abhiaiyer91 commented Apr 24, 2016

Thanks! I was going to do this but thanks for going for it!

@jbaxleyiii

This comment has been minimized.

Copy link
Member Author

jbaxleyiii commented Apr 24, 2016

@abhiaiyer91 I had some down time today 🎈

@stubailo stubailo self-assigned this Apr 25, 2016

@stubailo

This comment has been minimized.

Copy link
Member

stubailo commented Apr 25, 2016

I think this should be 0.2 to conform to NPM's semver handling!

@stubailo

This comment has been minimized.

Copy link
Member

stubailo commented Apr 25, 2016

I think we should add a warning in the case where the query returns a property that conflicts with other things we return like refetch, error, loading. It's easy to work around with an alias, but it would be great for people to find out about the problem early!

It would be a bit weird to have a root query called error, but who knows, especially as we add more metadata.

@abhiaiyer91

This comment has been minimized.

Copy link
Contributor

abhiaiyer91 commented Apr 25, 2016

Yes we can also write some Eslint rules for Apollo Query data, and ship that tooling?

@stubailo

This comment has been minimized.

Copy link
Member

stubailo commented Apr 25, 2016

Eslint rules for Apollo Query data

What would they do? We can add it here maybe: https://github.com/apollostack/eslint-plugin-graphql

Since a plugin can ship multiple rules, we can add as many as we want to that plugin.

@jbaxleyiii

This comment has been minimized.

Copy link
Member Author

jbaxleyiii commented Apr 25, 2016

@stubailo I'll update this PR tonight 🎉

James Baxley
add invariant warning if query or muation returns result with invalid…
… key, also correct semver version bump
@jbaxleyiii

This comment has been minimized.

Copy link
Member Author

jbaxleyiii commented Apr 26, 2016

@stubailo this one should be good to go. I'm good with calling it discussed and merging if you are

@stubailo stubailo merged commit 6bc5027 into master Apr 26, 2016

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
CLA Author has signed the Meteor CLA.
Details

@jbaxleyiii jbaxleyiii deleted the remove-result-key branch Jun 27, 2016

jbaxleyiii pushed a commit to apollographql/apollo-client that referenced this pull request Oct 17, 2017

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