-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
subscribeToMore for observableQuery #797
Changes from 4 commits
8ab63ce
77bc255
656c3ab
81a17da
4e433bb
181836c
e63b397
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,9 +2,10 @@ import { | |
ModifiableWatchQueryOptions, | ||
WatchQueryOptions, | ||
FetchMoreQueryOptions, | ||
SubscribeToMoreOptions, | ||
} from './watchQueryOptions'; | ||
|
||
import { Observable, Observer } from '../util/Observable'; | ||
import { Observable, Observer, Subscription } from '../util/Observable'; | ||
|
||
import { | ||
QueryScheduler, | ||
|
@@ -49,6 +50,7 @@ export class ObservableQuery extends Observable<ApolloQueryResult> { | |
private scheduler: QueryScheduler; | ||
private queryManager: QueryManager; | ||
private observers: Observer<ApolloQueryResult>[]; | ||
private subscriptionHandles: Subscription[]; | ||
|
||
private lastResult: ApolloQueryResult; | ||
private lastError: ApolloError; | ||
|
@@ -80,6 +82,7 @@ export class ObservableQuery extends Observable<ApolloQueryResult> { | |
this.queryId = queryId; | ||
this.shouldSubscribe = shouldSubscribe; | ||
this.observers = []; | ||
this.subscriptionHandles = []; | ||
} | ||
|
||
public result(): Promise<ApolloQueryResult> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When converting Observables to promises the convention is usually to take last value emitted, not first value emitted. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that should go as a comment on this PR, since that code was not modified at all. Perhaps open a new issue or a PR? But I'm not sure how following that convention will improve the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah you are right. (regarding commenting here)
|
||
|
@@ -165,7 +168,8 @@ export class ObservableQuery extends Observable<ApolloQueryResult> { | |
const reducer = fetchMoreOptions.updateQuery; | ||
const mapFn = (previousResult: any, { variables }: {variables: any }) => { | ||
|
||
// TODO REFACTOR: reached max recursion depth (figuratively). Continue renaming to variables further down when we have time. | ||
// TODO REF: reached max recursion depth (fig) when renaming queryVariables to variables. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think using abbreviations in comments is a good idea. |
||
// Continue renaming to variables further down when we have time. | ||
const queryVariables = variables; | ||
return reducer( | ||
previousResult, { | ||
|
@@ -178,6 +182,49 @@ export class ObservableQuery extends Observable<ApolloQueryResult> { | |
}); | ||
} | ||
|
||
// XXX the subscription variables are separate from the query variables. | ||
// if you want to update subscription variables, right now you have to do that separately, | ||
// and you can only do it by stopping the subscription and then subscribing again with new variables. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SGTM |
||
public subscribeToMore( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think it should return observable not just teardown function. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about it but decided against it, because I think if people want access to the subscription, they should use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so the whole point of this function is to update the store so the orginal observable will get updates? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's correct. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, that's pretty much it :) |
||
options: SubscribeToMoreOptions, | ||
): () => void { | ||
const observable = this.queryManager.startGraphQLSubscription({ | ||
document: options.document, | ||
variables: options.variables, | ||
}); | ||
|
||
const reducer = options.updateQuery; | ||
|
||
const subscription = observable.subscribe({ | ||
next: (subscriptionData) => { | ||
const mapFn = (previousResult: Object, { variables }: { variables: Object }) => { | ||
return reducer( | ||
previousResult, { | ||
subscriptionData, | ||
variables, | ||
} | ||
); | ||
}; | ||
this.updateQuery(mapFn); | ||
}, | ||
error: (err) => { | ||
// TODO implement something smart here when improving error handling | ||
console.error(err); | ||
}, | ||
}); | ||
|
||
this.subscriptionHandles.push(subscription); | ||
|
||
return () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we later want to return more stuff like the errors, we will wish we didn't just return a function here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the convention is to return observable, then right after use observer.error(error) |
||
// XXX technically we should also remove it from this.subscriptionHandles | ||
const i = this.subscriptionHandles.indexOf(subscription); | ||
if (i >= 0) { | ||
this.subscriptionHandles.splice(i, 1); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the above comment about removing it out of date? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it is indeed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. isn't splice removing it? |
||
subscription.unsubscribe(); | ||
} | ||
}; | ||
} | ||
|
||
public setOptions(opts: ModifiableWatchQueryOptions): Promise<ApolloQueryResult> { | ||
const oldOptions = this.options; | ||
this.options = assign({}, this.options, opts) as WatchQueryOptions; | ||
|
@@ -342,6 +389,9 @@ export class ObservableQuery extends Observable<ApolloQueryResult> { | |
this.scheduler.stopPollingQuery(this.queryId); | ||
} | ||
|
||
// stop all active GraphQL subscriptions | ||
this.subscriptionHandles.forEach( sub => sub.unsubscribe() ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i would re-init subscriptionHandlers into an empty array at that point... |
||
|
||
this.queryManager.stopQuery(this.queryId); | ||
this.observers = []; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,6 +80,15 @@ export interface FetchMoreQueryOptions { | |
variables?: { [key: string]: any }; | ||
} | ||
|
||
export type SubscribeToMoreOptions = { | ||
document: Document; | ||
variables?: { [key: string]: any }; | ||
updateQuery: (previousQueryResult: Object, options: { | ||
subscriptionData: any, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we just wrap this in a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i agree with you that both should be consistent.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, I wasn't sure where to do it. I actually did that in another place, but I'd rather solve it in the subscriptions themselves. Will do the "patching" for now. |
||
variables: { [key: string]: any }, | ||
}) => Object; | ||
} | ||
|
||
export interface DeprecatedSubscriptionOptions { | ||
query: Document; | ||
variables?: { [key: string]: any }; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
import * as chai from 'chai'; | ||
const { assert } = chai; | ||
|
||
import { | ||
mockSubscriptionNetworkInterface, | ||
} from './mocks/mockNetworkInterface'; | ||
import ApolloClient from '../src'; | ||
|
||
// import assign = require('lodash.assign'); | ||
// import clonedeep = require('lodash.clonedeep'); | ||
|
||
import gql from 'graphql-tag'; | ||
|
||
describe('subscribeToMore', () => { | ||
const query = gql` | ||
query aQuery { | ||
entry { | ||
value | ||
} | ||
} | ||
`; | ||
const result = { | ||
data: { | ||
entry: { | ||
value: 1, | ||
}, | ||
}, | ||
}; | ||
|
||
const req1 = { request: { query }, result }; | ||
|
||
const results = ['Dahivat Pandya', 'Amanda Liu'].map( | ||
name => ({ result: { name: name }, delay: 10 }) | ||
); | ||
|
||
const sub1 = { | ||
request: { | ||
query: gql` | ||
subscription newValues { | ||
name | ||
} | ||
`, | ||
}, | ||
id: 0, | ||
results: [...results], | ||
}; | ||
|
||
it('triggers new result from subscription data', (done) => { | ||
let latestResult: any = null; | ||
const networkInterface = mockSubscriptionNetworkInterface([sub1], req1); | ||
let counter = 0; | ||
|
||
const client = new ApolloClient({ | ||
networkInterface, | ||
addTypename: false, | ||
}); | ||
|
||
const obsHandle = client.watchQuery({ | ||
query, | ||
}); | ||
const sub = obsHandle.subscribe({ | ||
next(queryResult) { | ||
latestResult = queryResult; | ||
counter++; | ||
}, | ||
}); | ||
|
||
obsHandle.subscribeToMore({ | ||
document: gql` | ||
subscription newValues { | ||
name | ||
} | ||
`, | ||
updateQuery: (prev, { subscriptionData }) => { | ||
return { entry: { value: subscriptionData.name } }; | ||
}, | ||
}); | ||
|
||
setTimeout(() => { | ||
sub.unsubscribe(); | ||
assert.equal(counter, 3); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure exceptions will raise (and not just print) that way (setTimeout, annon func) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it worked alright. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cool, i have bad experience with timeouts and exceptions hehe.. |
||
assert.deepEqual(latestResult, { data: { entry: { value: 'Amanda Liu' } }, loading: false }); | ||
done(); | ||
}, 50); | ||
|
||
for (let i = 0; i < 2; i++) { | ||
networkInterface.fireResult(0); // 0 is the id of the subscription for the NI | ||
} | ||
}); | ||
|
||
// TODO add a test that checks that subscriptions are cancelled when obs is unsubscribed from. | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you're allowed to put spaces between the
**
like this in markdown.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's funny, because I think I copy-pasted it from another entry :D Will fix both.