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

Fixed inconsistency in naming of the prop for the subscription #1774

Merged
merged 2 commits into from Mar 16, 2018

Conversation

Projects
None yet
3 participants
@excitement-engineer
Copy link
Collaborator

excitement-engineer commented Mar 10, 2018

There is an inconsistency in the names of the props for the documents:

<Query query={} />
<Mutation mutation={} />
<Subscription query={} />

The name of the prop for the Subscription component is inconsistent with the others. Therefore, I propose that we call the prop subscription instead of query

<Subscription subscription={} />
@excitement-engineer

This comment has been minimized.

Copy link
Collaborator Author

excitement-engineer commented Mar 10, 2018

Curious to hear your thoughts @jbaxleyiii

@fbartho

This comment has been minimized.

Copy link

fbartho commented Mar 12, 2018

Counter-proposal: move the mutation prop to be query={. They're all GraphQL queries.

@excitement-engineer

This comment has been minimized.

Copy link
Collaborator Author

excitement-engineer commented Mar 15, 2018

This is also an option:) I'm happy as long as it is consistent!

@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Mar 16, 2018

I think we can either go query => operation or query => subscription. I think subscription will lead to the least confusion when looking back at old code, especially if you extend the component class and give it another name (like in TS)

@jbaxleyiii jbaxleyiii merged commit 76db01e into apollographql:master Mar 16, 2018

4 checks passed

CLA Author has signed the Meteor CLA.
Details
bundlesize ./dist/bundlesize.js: 10.54KB < maxSize 11KB (gzip)(7B larger than master, careful!)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 96.734%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment