Skip to content
This repository was archived by the owner on Apr 13, 2023. It is now read-only.

Commit d6e110b

Browse files
author
Tom Coleman
authored
Merge pull request #260 from apollostack/glasser/unsubscribe-crash
Fix crash in unsubscribeFromQuery
2 parents 6baf0fd + 8ee1a1f commit d6e110b

File tree

3 files changed

+213
-19
lines changed

3 files changed

+213
-19
lines changed

Changelog.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ Expect active development and potentially significant breaking changes in the `0
44

55
### vNext
66

7+
- Bug: Fix possible crash in unsubscribeFromQuery [#260](https://github.com/apollostack/react-apollo/pull/260)
8+
79
### v0.5.8
810

911
- Feature: Remove nested imports for apollo-client. Making local development eaiser. [#234](https://github.com/apollostack/react-apollo/pull/234)

src/graphql.tsx

Lines changed: 36 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,11 @@ export declare interface QueryOptions {
5454

5555
const defaultQueryData = {
5656
loading: true,
57-
errors: null,
57+
error: null,
58+
};
59+
const skippedQueryData = {
60+
loading: false,
61+
error: null,
5862
};
5963

6064
const defaultMapPropsToOptions = props => ({});
@@ -228,7 +232,8 @@ export default function graphql(
228232
private data: any = {}; // apollo data
229233
private type: DocumentType;
230234

231-
// request / action storage
235+
// request / action storage. Note that we delete querySubscription if we
236+
// unsubscribe but never delete queryObservable once it is created.
232237
private queryObservable: ObservableQuery | any;
233238
private querySubscription: Subscription;
234239

@@ -267,10 +272,13 @@ export default function graphql(
267272
}
268273

269274
componentWillReceiveProps(nextProps) {
270-
// if this has changed, remove data and unsubscribeFromQuery
271-
if (!mapPropsToSkip(this.props) && mapPropsToSkip(nextProps)) {
272-
delete this.data;
273-
return this.unsubscribeFromQuery();
275+
if (mapPropsToSkip(nextProps)) {
276+
if (!mapPropsToSkip(this.props)) {
277+
// if this has changed, remove data and unsubscribeFromQuery
278+
this.data = assign({}, skippedQueryData) as any;
279+
this.unsubscribeFromQuery();
280+
}
281+
return;
274282
}
275283
if (shallowEqual(this.props, nextProps)) return;
276284

@@ -313,11 +321,11 @@ export default function graphql(
313321
// Create the observable but don't subscribe yet. The query won't
314322
// fire until we do.
315323
const opts: QueryOptions = this.calculateOptions(this.props);
316-
this.data = assign({}, defaultQueryData) as any;
317324

318325
if (opts.skip) {
319-
this.data.loading = false;
326+
this.data = assign({}, skippedQueryData) as any;
320327
} else {
328+
this.data = assign({}, defaultQueryData) as any;
321329
this.createQuery(opts);
322330
}
323331
}
@@ -333,17 +341,21 @@ export default function graphql(
333341
}, opts));
334342
}
335343

336-
assign(this.data, observableQueryFields(this.queryObservable));
344+
this.initializeData(opts);
345+
}
337346

338-
if (!opts.forceFetch && this.type !== DocumentType.Subscription) {
339-
const currentResult = this.queryObservable.currentResult();
340-
// try and fetch initial data from the store
341-
assign(this.data, currentResult.data, { loading: currentResult.loading });
342-
}
347+
initializeData(opts: QueryOptions) {
348+
assign(this.data, observableQueryFields(this.queryObservable));
343349

344350
if (this.type === DocumentType.Subscription) {
345351
opts = this.calculateOptions(this.props, opts);
346352
assign(this.data, { loading: true }, { variables: opts.variables });
353+
} else if (!opts.forceFetch) {
354+
const currentResult = this.queryObservable.currentResult();
355+
// try and fetch initial data from the store
356+
assign(this.data, currentResult.data, { loading: currentResult.loading });
357+
} else {
358+
assign(this.data, { loading: true });
347359
}
348360
}
349361

@@ -353,8 +365,8 @@ export default function graphql(
353365
if (opts.skip) {
354366
if (this.querySubscription) {
355367
this.hasOperationDataChanged = true;
356-
this.data = { loading: false };
357-
this.querySubscription.unsubscribe();
368+
this.data = assign({}, skippedQueryData) as any;
369+
this.unsubscribeFromQuery();
358370
this.forceRenderChildren();
359371
}
360372
return;
@@ -377,13 +389,18 @@ export default function graphql(
377389
// if we skipped initially, we may not have yet created the observable
378390
if (!this.queryObservable) {
379391
this.createQuery(opts);
392+
} else if (!this.data.refetch) {
393+
// we've run this query before, but then we've skipped it (resetting
394+
// data to skippedQueryData) and now we're unskipping it. Make sure
395+
// the data fields are set as if we hadn't run it.
396+
this.initializeData(opts);
380397
}
381398

382399
const next = (results: any) => {
383400
if (this.type === DocumentType.Subscription) {
384401
results = { data: results, loading: false, error: null };
385402
}
386-
const { data, loading, error } = results;
403+
const { data, loading, error = null } = results;
387404
const clashingKeys = Object.keys(observableQueryFields(data));
388405
invariant(clashingKeys.length === 0,
389406
`the result of the '${graphQLDisplayName}' operation contains keys that ` +
@@ -416,9 +433,9 @@ export default function graphql(
416433
}
417434

418435
unsubscribeFromQuery() {
419-
if ((this.querySubscription as Subscription).unsubscribe) {
436+
if (this.querySubscription) {
420437
(this.querySubscription as Subscription).unsubscribe();
421-
delete this.queryObservable;
438+
delete this.querySubscription;
422439
}
423440
}
424441

test/react-web/client/graphql/queries-1.test.tsx

Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -370,6 +370,43 @@ describe('queries', () => {
370370
}, 25);
371371
});
372372

373+
it('continues to not subscribe to a skipped query when props change', (done) => {
374+
const query = gql`query people { allPeople(first: 1) { people { name } } }`;
375+
const networkInterface = mockNetworkInterface();
376+
const oldQuery = networkInterface.query;
377+
378+
networkInterface.query = function (request) {
379+
fail(new Error('query ran even though skip present'));
380+
return oldQuery.call(this, request);
381+
};
382+
const client = new ApolloClient({ networkInterface });
383+
384+
@graphql(query, { skip: true })
385+
class Container extends React.Component<any, any> {8
386+
componentWillReceiveProps(props) {
387+
done();
388+
}
389+
render() {
390+
return null;
391+
}
392+
};
393+
394+
class Parent extends React.Component<any, any> {
395+
constructor() {
396+
super();
397+
this.state = { foo: 42 };
398+
}
399+
componentDidMount() {
400+
this.setState({ foo: 43 });
401+
}
402+
render() {
403+
return <Container foo={this.state.foo} />;
404+
}
405+
};
406+
407+
renderer.create(<ProviderMock client={client}><Parent /></ProviderMock>);
408+
});
409+
373410
it('doesn\'t run options or props when skipped', (done) => {
374411
const query = gql`query people { allPeople(first: 1) { people { name } } }`;
375412
const data = { allPeople: { people: [ { name: 'Luke Skywalker' } ] } };
@@ -426,6 +463,106 @@ describe('queries', () => {
426463
}, 25);
427464
});
428465

466+
// test the case of skip:false -> skip:true -> skip:false to make sure things
467+
// are cleaned up properly
468+
it('allows you to skip then unskip a query with top-level syntax', (done) => {
469+
const query = gql`query people { allPeople(first: 1) { people { name } } }`;
470+
const data = { allPeople: { people: [ { name: 'Luke Skywalker' } ] } };
471+
const networkInterface = mockNetworkInterface({ request: { query }, result: { data } });
472+
const client = new ApolloClient({ networkInterface });
473+
474+
let hasSkipped = false;
475+
@graphql(query, { skip: ({ skip }) => skip })
476+
class Container extends React.Component<any, any> {8
477+
componentWillReceiveProps(newProps) {
478+
if (newProps.skip) {
479+
hasSkipped = true;
480+
this.props.setSkip(false);
481+
} else {
482+
if (hasSkipped) {
483+
done();
484+
} else {
485+
this.props.setSkip(true);
486+
}
487+
}
488+
}
489+
render() {
490+
return null;
491+
}
492+
};
493+
494+
class Parent extends React.Component<any, any> {
495+
constructor() {
496+
super();
497+
this.state = { skip: false };
498+
}
499+
render() {
500+
return <Container skip={this.state.skip} setSkip={(skip) => this.setState({ skip })} />;
501+
}
502+
};
503+
504+
renderer.create(<ProviderMock client={client}><Parent /></ProviderMock>);
505+
});
506+
507+
it('allows you to skip then unskip a query with opts syntax', (done) => {
508+
const query = gql`query people { allPeople(first: 1) { people { name } } }`;
509+
const data = { allPeople: { people: [ { name: 'Luke Skywalker' } ] } };
510+
const nextData = { allPeople: { people: [ { name: 'Anakin Skywalker' } ] } };
511+
const networkInterface = mockNetworkInterface({
512+
request: { query }, result: { data }, newData: () => ({ data: nextData }) });
513+
const oldQuery = networkInterface.query;
514+
515+
let ranQuery = 0;
516+
networkInterface.query = function (request) {
517+
ranQuery++;
518+
return oldQuery.call(this, request);
519+
};
520+
const client = new ApolloClient({ networkInterface });
521+
522+
let hasSkipped = false;
523+
let hasRequeried = false;
524+
@graphql(query, { options: ({ skip }) => ({ skip, forceFetch: true }) })
525+
class Container extends React.Component<any, any> {8
526+
componentWillReceiveProps(newProps) {
527+
if (newProps.skip) {
528+
// Step 2. We shouldn't query again.
529+
expect(ranQuery).toBe(1);
530+
hasSkipped = true;
531+
this.props.setSkip(false);
532+
} else if (hasRequeried) {
533+
// Step 4. We need to actually get the data from the query into the component!
534+
expect(newProps.data.loading).toBe(false);
535+
done();
536+
} else if (hasSkipped) {
537+
// Step 3. We need to query again!
538+
expect(newProps.data.loading).toBe(true);
539+
expect(ranQuery).toBe(2);
540+
hasRequeried = true;
541+
} else {
542+
// Step 1. We've queried once.
543+
expect(ranQuery).toBe(1);
544+
this.props.setSkip(true);
545+
}
546+
}
547+
render() {
548+
return null;
549+
}
550+
};
551+
552+
class Parent extends React.Component<any, any> {
553+
constructor() {
554+
super();
555+
this.state = { skip: false };
556+
}
557+
render() {
558+
return <Container skip={this.state.skip} setSkip={(skip) => this.setState({ skip })} />;
559+
}
560+
};
561+
562+
renderer.create(<ProviderMock client={client}><Parent /></ProviderMock>);
563+
});
564+
565+
429566
it('removes the injected props if skip becomes true', (done) => {
430567
let count = 0;
431568
const query = gql`
@@ -495,6 +632,44 @@ describe('queries', () => {
495632
renderer.create(<ProviderMock client={client}><ChangingProps /></ProviderMock>);
496633
});
497634

635+
636+
it('allows you to unmount a skipped query', (done) => {
637+
const query = gql`query people { allPeople(first: 1) { people { name } } }`;
638+
const networkInterface = mockNetworkInterface();
639+
const client = new ApolloClient({ networkInterface });
640+
641+
@graphql(query, {
642+
skip: true,
643+
})
644+
class Container extends React.Component<any, any> {
645+
componentDidMount() {
646+
this.props.hide();
647+
}
648+
componentWillUnmount() {
649+
done();
650+
}
651+
render() {
652+
return null;
653+
}
654+
};
655+
656+
class Hider extends React.Component<any, any> {
657+
constructor() {
658+
super();
659+
this.state = { hide: false };
660+
}
661+
render() {
662+
if (this.state.hide) {
663+
return null;
664+
}
665+
return <Container hide={() => this.setState({ hide: true })} />;
666+
}
667+
}
668+
669+
renderer.create(<ProviderMock client={client}><Hider /></ProviderMock>);
670+
});
671+
672+
498673
it('reruns the query if it changes', (done) => {
499674
let count = 0;
500675
const query = gql`

0 commit comments

Comments
 (0)