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

Fix crash in unsubscribeFromQuery #260

Merged
merged 1 commit into from
Oct 11, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ Expect active development and potentially significant breaking changes in the `0

### vNext

- Bug: Fix possible crash in unsubscribeFromQuery [#260](https://github.com/apollostack/react-apollo/pull/260)

### v0.5.8

- Feature: Remove nested imports for apollo-client. Making local development eaiser. [#234](https://github.com/apollostack/react-apollo/pull/234)
Expand Down
55 changes: 36 additions & 19 deletions src/graphql.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,11 @@ export declare interface QueryOptions {

const defaultQueryData = {
loading: true,
errors: null,
error: null,
};
const skippedQueryData = {
loading: false,
error: null,
};

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

// request / action storage
// request / action storage. Note that we delete querySubscription if we
// unsubscribe but never delete queryObservable once it is created.
private queryObservable: ObservableQuery | any;
private querySubscription: Subscription;

Expand Down Expand Up @@ -267,10 +272,13 @@ export default function graphql(
}

componentWillReceiveProps(nextProps) {
// if this has changed, remove data and unsubscribeFromQuery
if (!mapPropsToSkip(this.props) && mapPropsToSkip(nextProps)) {
delete this.data;
return this.unsubscribeFromQuery();
if (mapPropsToSkip(nextProps)) {
if (!mapPropsToSkip(this.props)) {
// if this has changed, remove data and unsubscribeFromQuery
this.data = assign({}, skippedQueryData) as any;
this.unsubscribeFromQuery();
}
return;
}
if (shallowEqual(this.props, nextProps)) return;

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

if (opts.skip) {
this.data.loading = false;
this.data = assign({}, skippedQueryData) as any;
} else {
this.data = assign({}, defaultQueryData) as any;
this.createQuery(opts);
}
}
Expand All @@ -333,17 +341,21 @@ export default function graphql(
}, opts));
}

assign(this.data, observableQueryFields(this.queryObservable));
this.initializeData(opts);
}

if (!opts.forceFetch && this.type !== DocumentType.Subscription) {
const currentResult = this.queryObservable.currentResult();
// try and fetch initial data from the store
assign(this.data, currentResult.data, { loading: currentResult.loading });
}
initializeData(opts: QueryOptions) {
assign(this.data, observableQueryFields(this.queryObservable));

if (this.type === DocumentType.Subscription) {
opts = this.calculateOptions(this.props, opts);
assign(this.data, { loading: true }, { variables: opts.variables });
} else if (!opts.forceFetch) {
const currentResult = this.queryObservable.currentResult();
// try and fetch initial data from the store
assign(this.data, currentResult.data, { loading: currentResult.loading });
} else {
assign(this.data, { loading: true });
}
}

Expand All @@ -353,8 +365,8 @@ export default function graphql(
if (opts.skip) {
if (this.querySubscription) {
this.hasOperationDataChanged = true;
this.data = { loading: false };
this.querySubscription.unsubscribe();
this.data = assign({}, skippedQueryData) as any;
this.unsubscribeFromQuery();
this.forceRenderChildren();
}
return;
Expand All @@ -377,13 +389,18 @@ export default function graphql(
// if we skipped initially, we may not have yet created the observable
if (!this.queryObservable) {
this.createQuery(opts);
} else if (!this.data.refetch) {
// we've run this query before, but then we've skipped it (resetting
// data to skippedQueryData) and now we're unskipping it. Make sure
// the data fields are set as if we hadn't run it.
this.initializeData(opts);
}

const next = (results: any) => {
if (this.type === DocumentType.Subscription) {
results = { data: results, loading: false, error: null };
}
const { data, loading, error } = results;
const { data, loading, error = null } = results;
const clashingKeys = Object.keys(observableQueryFields(data));
invariant(clashingKeys.length === 0,
`the result of the '${graphQLDisplayName}' operation contains keys that ` +
Expand Down Expand Up @@ -416,9 +433,9 @@ export default function graphql(
}

unsubscribeFromQuery() {
if ((this.querySubscription as Subscription).unsubscribe) {
if (this.querySubscription) {
(this.querySubscription as Subscription).unsubscribe();
delete this.queryObservable;
delete this.querySubscription;
}
}

Expand Down
175 changes: 175 additions & 0 deletions test/react-web/client/graphql/queries-1.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,43 @@ describe('queries', () => {
}, 25);
});

it('continues to not subscribe to a skipped query when props change', (done) => {
const query = gql`query people { allPeople(first: 1) { people { name } } }`;
const networkInterface = mockNetworkInterface();
const oldQuery = networkInterface.query;

networkInterface.query = function (request) {
fail(new Error('query ran even though skip present'));
Copy link
Member Author

@glasser glasser Oct 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't figure out what fail is. It's not in https://facebook.github.io/jest/docs/api.html or defined in the file. But it does seem to work (I tested with the change reverted...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess fail is a Jasmine thing. Jest's docs seem a bit incomplete.

return oldQuery.call(this, request);
};
const client = new ApolloClient({ networkInterface });

@graphql(query, { skip: true })
class Container extends React.Component<any, any> {8
componentWillReceiveProps(props) {
done();
}
render() {
return null;
}
};

class Parent extends React.Component<any, any> {
constructor() {
super();
this.state = { foo: 42 };
}
componentDidMount() {
this.setState({ foo: 43 });
}
render() {
return <Container foo={this.state.foo} />;
}
};

renderer.create(<ProviderMock client={client}><Parent /></ProviderMock>);
});

it('doesn\'t run options or props when skipped', (done) => {
const query = gql`query people { allPeople(first: 1) { people { name } } }`;
const data = { allPeople: { people: [ { name: 'Luke Skywalker' } ] } };
Expand Down Expand Up @@ -426,6 +463,106 @@ describe('queries', () => {
}, 25);
});

// test the case of skip:false -> skip:true -> skip:false to make sure things
// are cleaned up properly
it('allows you to skip then unskip a query with top-level syntax', (done) => {
const query = gql`query people { allPeople(first: 1) { people { name } } }`;
const data = { allPeople: { people: [ { name: 'Luke Skywalker' } ] } };
const networkInterface = mockNetworkInterface({ request: { query }, result: { data } });
const client = new ApolloClient({ networkInterface });

let hasSkipped = false;
@graphql(query, { skip: ({ skip }) => skip })
class Container extends React.Component<any, any> {8
componentWillReceiveProps(newProps) {
if (newProps.skip) {
hasSkipped = true;
this.props.setSkip(false);
} else {
if (hasSkipped) {
done();
} else {
this.props.setSkip(true);
}
}
}
render() {
return null;
}
};

class Parent extends React.Component<any, any> {
constructor() {
super();
this.state = { skip: false };
}
render() {
return <Container skip={this.state.skip} setSkip={(skip) => this.setState({ skip })} />;
}
};

renderer.create(<ProviderMock client={client}><Parent /></ProviderMock>);
});

it('allows you to skip then unskip a query with opts syntax', (done) => {
Copy link
Member Author

@glasser glasser Oct 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one wasn't actually failing (without the code change). Or maybe it was failing if you only reverted one bit of the code change but not the whole commit.

I've made it longer and more detailed, but then it doesn't actually pass any more: looks like I can't convince it to run the query again. Maybe that's due to some caching and that's a feature? I guess @tmeasday is asleep now; @jbaxleyiii want to take a look at my version of this test and help me figure out how to get it to fail? The goal is to trigger a now-fixed bug where the check on opts.skip in subscribeToQuery doesn't delete querySubscription and thus never re-subscribes when unskipped.

const query = gql`query people { allPeople(first: 1) { people { name } } }`;
const data = { allPeople: { people: [ { name: 'Luke Skywalker' } ] } };
const nextData = { allPeople: { people: [ { name: 'Anakin Skywalker' } ] } };
const networkInterface = mockNetworkInterface({
request: { query }, result: { data }, newData: () => ({ data: nextData }) });
const oldQuery = networkInterface.query;

let ranQuery = 0;
networkInterface.query = function (request) {
ranQuery++;
return oldQuery.call(this, request);
};
const client = new ApolloClient({ networkInterface });

let hasSkipped = false;
let hasRequeried = false;
@graphql(query, { options: ({ skip }) => ({ skip, forceFetch: true }) })
class Container extends React.Component<any, any> {8
componentWillReceiveProps(newProps) {
if (newProps.skip) {
// Step 2. We shouldn't query again.
expect(ranQuery).toBe(1);
hasSkipped = true;
this.props.setSkip(false);
} else if (hasRequeried) {
// Step 4. We need to actually get the data from the query into the component!
expect(newProps.data.loading).toBe(false);
done();
} else if (hasSkipped) {
// Step 3. We need to query again!
expect(newProps.data.loading).toBe(true);
expect(ranQuery).toBe(2);
hasRequeried = true;
} else {
// Step 1. We've queried once.
expect(ranQuery).toBe(1);
this.props.setSkip(true);
}
}
render() {
return null;
}
};

class Parent extends React.Component<any, any> {
constructor() {
super();
this.state = { skip: false };
}
render() {
return <Container skip={this.state.skip} setSkip={(skip) => this.setState({ skip })} />;
}
};

renderer.create(<ProviderMock client={client}><Parent /></ProviderMock>);
});


it('removes the injected props if skip becomes true', (done) => {
let count = 0;
const query = gql`
Expand Down Expand Up @@ -495,6 +632,44 @@ describe('queries', () => {
renderer.create(<ProviderMock client={client}><ChangingProps /></ProviderMock>);
});


it('allows you to unmount a skipped query', (done) => {
const query = gql`query people { allPeople(first: 1) { people { name } } }`;
const networkInterface = mockNetworkInterface();
const client = new ApolloClient({ networkInterface });

@graphql(query, {
skip: true,
})
class Container extends React.Component<any, any> {
componentDidMount() {
this.props.hide();
}
componentWillUnmount() {
done();
}
render() {
return null;
}
};

class Hider extends React.Component<any, any> {
constructor() {
super();
this.state = { hide: false };
}
render() {
if (this.state.hide) {
return null;
}
return <Container hide={() => this.setState({ hide: true })} />;
}
}

renderer.create(<ProviderMock client={client}><Hider /></ProviderMock>);
});


it('reruns the query if it changes', (done) => {
let count = 0;
const query = gql`
Expand Down