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

v2.1 Implementation of Subscription component #1483

Merged
merged 9 commits into from Dec 30, 2017

Conversation

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

excitement-engineer commented Dec 28, 2017

Implementation of the Subscription component for the next version of react-apollo.

@rosskevin

This comment has been minimized.

Copy link
Collaborator

rosskevin commented Dec 28, 2017

Noun or verb?

<Subscription subscription={subscription}>

or

<Subscribe subscription={subscription}>

I'm using Query now and I don't like the repetitiveness of the DocumentNode prop of the same name as the component. Same here with subscription. I wonder if we should choose something simpler/less verbose.

I have no idea what is generic, but this illustrates where I'm going:

<Subscribe node={subscripton}>
<Query node={query}>
@excitement-engineer

This comment has been minimized.

Copy link
Collaborator Author

excitement-engineer commented Dec 29, 2017

I'm in favour of sticking to the exact language used by graphql for the component: Query, Mutation and Subscription to prevent any confusion, what do you think?

A node does not intuitively signal a DocumentNode to me, should we simply call the prop query?

<Query query={...} />
<Subscription query={...} />
@rosskevin

This comment has been minimized.

Copy link
Collaborator

rosskevin commented Dec 29, 2017

I'm not sure. Using query is a length improvement, and I think an accurate name (not positive though). Either way I want to reduce length without reducing explicitness or clarity.

@excitement-engineer excitement-engineer changed the title [WIP] Implementation of Subscription component v2.1 Implementation of Subscription component Dec 30, 2017

excitement-engineer added some commits Dec 30, 2017

@meteor-bot

This comment has been minimized.

Copy link

meteor-bot commented Dec 30, 2017

Warnings
⚠️

❗️ Big PR

⚠️

There are library changes, but not tests. That's OK as long as you're refactoring existing code

Generated by 🚫 dangerJS

@rosskevin
Copy link
Collaborator

rosskevin left a comment

Only one change on type. The rest is really just style preferences which are more or less personal choices - no real reason I'm the authority that says it must be different 😄

children: (result: any) => React.ReactNode;
}

export interface SubscriptionState<TData = any> {

This comment has been minimized.

@rosskevin

rosskevin Dec 30, 2017

Collaborator

Remove export. I just call these State since they are essentially private/not exported.

This comment has been minimized.

@excitement-engineer

excitement-engineer Dec 30, 2017

Author Collaborator

I tried this but typescript doesn't allow it with error:

'extends' clause of exported class 'Subscription' has or is using private name 'SubscriptionState'.

This comment has been minimized.

@rosskevin

rosskevin Dec 30, 2017

Collaborator

ok, if TS is going to complain and require the export (an obscure open issue in TS), then it is best to leave it as-is.


private client: ApolloClient<any>;
//TODO: What is the type here?
private queryObservable: any;

This comment has been minimized.

@rosskevin

rosskevin Dec 30, 2017

Collaborator

From ApolloClient.d.ts (your IDE should allow you to follow the declaration of client.subscribe

import {Observable} from 'apollo-client/util/Observable'
Observable<any>

This comment has been minimized.

@excitement-engineer

excitement-engineer Dec 30, 2017

Author Collaborator

Great thanks!

this.initialize(nextProps);
this.startSubscription();
this.setState(this.getInitialState(nextProps));
}

This comment has been minimized.

@rosskevin

rosskevin Dec 30, 2017

Collaborator

My preference in this section is that it is unnecessarily broken up. initialize and getInitialState are not reused, so i would simply bring their implementation up here. It increases comprehension without increasing complexity.

I could even say the same about start/end subscription - but I would have to see it before making a judgement.

My go-to in this case would probably be all that code right here. All init code, not reused, fairly simple, and if needed, a one-line comment to break it up.

Reasoning: when I see code in a method that is one-line I think - this must be reused, where is it reused and why?

This comment has been minimized.

@excitement-engineer

excitement-engineer Dec 30, 2017

Author Collaborator

I'm not sure I understand? All of these methods are being reused in the component

This comment has been minimized.

@rosskevin

rosskevin Dec 30, 2017

Collaborator

ah, ok good. It would have been easier for me to spot in my IDE, but you are right, these are needed.

@excitement-engineer excitement-engineer merged commit 2341454 into apollographql:master Dec 30, 2017

4 checks passed

CLA Author has signed the Meteor CLA.
Details
bundlesize ./lib/umd/react-apollo.js: 4.66KB < maxSize 4.7KB gzip (same as master)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.08%) to 96.181%
Details

@excitement-engineer excitement-engineer deleted the excitement-engineer:subscription-component branch Dec 30, 2017

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