Skip to content

Commit

Permalink
Fix for synchronous callbacks
Browse files Browse the repository at this point in the history 
Summary: a2ce9e1 made it so callbacks may be called synchronously (if the data is cached). In such a case, `onCompleted`, `onNext` and `onError` within `ReactRelayPaginationContainer` and `ReactRelayRefetchContainer` can be called before `environment.streamQuery` returns. The code currently hinges on `this._pendingRefetch` being set before the callbacks execute. This commit makes it so we don't rely on that anymore!

Reviewed By: yuzhi

Differential Revision: D5484321

fbshipit-source-id: 1b0eadfae814e852ef29e9bf7e683ff5226cb898
  • Loading branch information
@pranaygp @facebook-github-bot
pranaygp authored and facebook-github-bot committed Jul 26, 2017
1 parent ec29583 commit 2ffb5c6
Show file tree
Hide file tree
Showing 5 changed files with 190 additions and 9 deletions.
14 changes: 13 additions & 1 deletion packages/react-relay/modern/ReactRelayPaginationContainer.js
View file
Expand Up @@ -324,6 +324,7 @@ function createContainerWithFragments<TConfig, TClass: ReactClass<TConfig>>(

class Container extends React.Component {
state: ContainerState;
_isARequestInFlight: boolean;
_localVariables: ?Variables;
_pendingRefetch: ?Disposable;
_references: Array<Disposable>;
Expand All @@ -334,6 +335,7 @@ function createContainerWithFragments<TConfig, TClass: ReactClass<TConfig>>(
super(props, context);
const relay = assertRelayContext(context.relay);
const {createFragmentSpecResolver} = relay.environment.unstable_internal;
this._isARequestInFlight = false;
this._localVariables = null;
this._pendingRefetch = null;
this._references = [];
Expand Down Expand Up @@ -623,6 +625,7 @@ function createContainerWithFragments<TConfig, TClass: ReactClass<TConfig>>(

const onCompleted = () => {
this._pendingRefetch = null;
this._isARequestInFlight = false;
this._relayContext = {
environment: this.context.relay.environment,
variables: {
Expand Down Expand Up @@ -656,6 +659,7 @@ function createContainerWithFragments<TConfig, TClass: ReactClass<TConfig>>(
};
const onError = error => {
this._pendingRefetch = null;
this._isARequestInFlight = false;
callback && callback(error);
};

Expand All @@ -667,13 +671,19 @@ function createContainerWithFragments<TConfig, TClass: ReactClass<TConfig>>(
if (this._pendingRefetch) {
this._pendingRefetch.dispose();
}

this._isARequestInFlight = true;
const pendingRefetch = environment.streamQuery({
cacheConfig,
onCompleted,
onError,
operation,
});
this._pendingRefetch = pendingRefetch;
if (this._isARequestInFlight) {
this._pendingRefetch = pendingRefetch;
} else {
this._pendingRefetch = null;
}
return {
dispose: () => {
// Disposing a loadMore() call should always dispose the fetch itself,
Expand All @@ -682,6 +692,7 @@ function createContainerWithFragments<TConfig, TClass: ReactClass<TConfig>>(
pendingRefetch.dispose();
if (this._pendingRefetch === pendingRefetch) {
this._pendingRefetch = null;
this._isARequestInFlight = false;
}
},
};
Expand All @@ -694,6 +705,7 @@ function createContainerWithFragments<TConfig, TClass: ReactClass<TConfig>>(
if (this._pendingRefetch) {
this._pendingRefetch.dispose();
this._pendingRefetch = null;
this._isARequestInFlight = false;
}
}

Expand Down
15 changes: 13 additions & 2 deletions packages/react-relay/modern/ReactRelayRefetchContainer.js
View file
Expand Up @@ -64,6 +64,7 @@ function createContainerWithFragments<TConfig, TClass: ReactClass<TConfig>>(

class Container extends React.Component {
state: ContainerState;
_isARequestInFlight: boolean;
_localVariables: ?Variables;
_pendingRefetch: ?Disposable;
_references: Array<Disposable>;
Expand All @@ -74,6 +75,7 @@ function createContainerWithFragments<TConfig, TClass: ReactClass<TConfig>>(
super(props, context);
const relay = assertRelayContext(context.relay);
const {createFragmentSpecResolver} = relay.environment.unstable_internal;
this._isARequestInFlight = false;
this._localVariables = null;
this._pendingRefetch = null;
this._references = [];
Expand Down Expand Up @@ -177,6 +179,7 @@ function createContainerWithFragments<TConfig, TClass: ReactClass<TConfig>>(
if (this._pendingRefetch) {
this._pendingRefetch.dispose();
this._pendingRefetch = null;
this._isARequestInFlight = false;
}
}

Expand Down Expand Up @@ -229,12 +232,13 @@ function createContainerWithFragments<TConfig, TClass: ReactClass<TConfig>>(
: fetchVariables;

const onNext = response => {
if (!this._pendingRefetch) {
if (!this._isARequestInFlight) {
// only call callback once per refetch
return;
}
// TODO t15106389: add helper utility for fetching more data
this._pendingRefetch = null;
this._isARequestInFlight = false;
this._relayContext = {
environment: this.context.relay.environment,
variables: fragmentVariables,
Expand All @@ -247,6 +251,7 @@ function createContainerWithFragments<TConfig, TClass: ReactClass<TConfig>>(
};
const onError = error => {
this._pendingRefetch = null;
this._isARequestInFlight = false;
callback && callback(error);
};

Expand All @@ -267,13 +272,18 @@ function createContainerWithFragments<TConfig, TClass: ReactClass<TConfig>>(
if (this._pendingRefetch) {
this._pendingRefetch.dispose();
}
this._isARequestInFlight = true;
const pendingRefetch = environment.streamQuery({
cacheConfig,
onError,
onNext,
operation,
});
this._pendingRefetch = pendingRefetch;
if (this._isARequestInFlight) {
this._pendingRefetch = pendingRefetch;
} else {
this._pendingRefetch = null;
}
return {
dispose: () => {
// Disposing a refetch() call should always dispose the fetch itself,
Expand All @@ -282,6 +292,7 @@ function createContainerWithFragments<TConfig, TClass: ReactClass<TConfig>>(
pendingRefetch.dispose();
if (this._pendingRefetch === pendingRefetch) {
this._pendingRefetch = null;
this._isARequestInFlight = false;
}
},
};
Expand Down
88 changes: 85 additions & 3 deletions packages/react-relay/modern/__tests__/ReactRelayPaginationContainer-test.js
View file
Expand Up @@ -18,14 +18,15 @@ const ReactRelayPropTypes = require('ReactRelayPropTypes');
const ReactTestRenderer = require('ReactTestRenderer');
const RelayConnectionHandler = require('RelayConnectionHandler');
const RelayModernTestUtils = require('RelayModernTestUtils');
const {ROOT_ID} = require('RelayStoreUtils');
const {createMockEnvironment} = require('RelayModernMockEnvironment');
const {createOperationSelector} = require('RelayModernOperationSelector');

const {
END_CURSOR,
HAS_NEXT_PAGE,
PAGE_INFO,
} = require('RelayConnectionInterface');
const {createMockEnvironment} = require('RelayModernMockEnvironment');
const {createOperationSelector} = require('RelayModernOperationSelector');
const {ROOT_ID} = require('RelayStoreUtils');
const {generateAndCompile} = RelayModernTestUtils;

describe('ReactRelayPaginationContainer', () => {
Expand Down Expand Up @@ -963,6 +964,7 @@ describe('ReactRelayPaginationContainer', () => {
node: UserQuery.fragment,
variables,
}).data.node;
environment.mock.clearCache();
ReactTestRenderer.create(
<ContextSetter environment={environment} variables={variables}>
<TestContainer user={userPointer} />
Expand Down Expand Up @@ -1000,6 +1002,45 @@ describe('ReactRelayPaginationContainer', () => {
});
expect(isLoading()).toBe(false);
});

it('returns false if a cached response exists', () => {
environment.mock.cachePayload(
UserQuery,
{
after: 'cursor:1',
count: 1,
id: '4',
orderby: ['name'],
},
{
data: {
node: {
id: '4',
__typename: 'User',
friends: {
edges: [
{
cursor: 'cursor:2',
node: {
__typename: 'User',
id: 'node:2',
},
},
],
pageInfo: {
endCursor: 'cursor:2',
hasNextPage: true,
hasPreviousPage: false,
startCursor: 'cursor:2',
},
},
},
},
},
);
loadMore(1, jest.fn());
expect(isLoading()).toBe(false);
});
});

describe('loadMore()', () => {
Expand All @@ -1019,6 +1060,7 @@ describe('ReactRelayPaginationContainer', () => {
node: UserQuery.fragment,
variables,
}).data.node;
environment.mock.clearCache();
instance = ReactTestRenderer.create(
<ContextSetter environment={environment} variables={variables}>
<TestContainer user={userPointer} />
Expand Down Expand Up @@ -1158,6 +1200,46 @@ describe('ReactRelayPaginationContainer', () => {
expect(callback).toBeCalledWith(error);
});

it('calls the callback even if a cached response exists', () => {
environment.mock.cachePayload(
UserQuery,
{
after: 'cursor:1',
count: 1,
id: '4',
orderby: ['name'],
},
{
data: {
node: {
id: '4',
__typename: 'User',
friends: {
edges: [
{
cursor: 'cursor:2',
node: {
__typename: 'User',
id: 'node:2',
},
},
],
pageInfo: {
endCursor: 'cursor:2',
hasNextPage: true,
hasPreviousPage: false,
startCursor: 'cursor:2',
},
},
},
},
},
);
const callback = jest.fn();
loadMore(1, callback);
expect(callback).toHaveBeenCalled();
});

it('renders with the results of the new variables on success', async () => {
expect.assertions(5);
expect(render.mock.calls.length).toBe(1);
Expand Down
48 changes: 45 additions & 3 deletions packages/react-relay/modern/__tests__/ReactRelayRefetchContainer-test.js
View file
Expand Up @@ -13,13 +13,13 @@
'use strict';

const React = require('React');
const ReactRelayRefetchContainer = require('ReactRelayRefetchContainer');
const ReactRelayPropTypes = require('ReactRelayPropTypes');
const ReactRelayRefetchContainer = require('ReactRelayRefetchContainer');
const ReactTestRenderer = require('ReactTestRenderer');
const {createMockEnvironment} = require('RelayModernMockEnvironment');
const RelayModernTestUtils = require('RelayModernTestUtils');
const {createOperationSelector} = require('RelayModernOperationSelector');

const {createMockEnvironment} = require('RelayModernMockEnvironment');
const {createOperationSelector} = require('RelayModernOperationSelector');
const {ROOT_ID} = require('RelayStoreUtils');

describe('ReactRelayRefetchContainer', () => {
Expand Down Expand Up @@ -561,6 +561,7 @@ describe('ReactRelayRefetchContainer', () => {
node: UserQuery.fragment,
variables: {id: '4'},
}).data.node;
environment.mock.clearCache();
instance = ReactTestRenderer.create(
<ContextSetter environment={environment} variables={variables}>
<TestContainer user={userPointer} />
Expand Down Expand Up @@ -622,6 +623,47 @@ describe('ReactRelayRefetchContainer', () => {
expect(callback).toBeCalledWith(error);
});

it('calls the callback even if the response is cached', () => {
const refetchVariables = {
cond: false,
id: '4',
};
const fetchedVariables = {id: '4'};
environment.mock.cachePayload(UserQuery, fetchedVariables, {
data: {
node: {
id: '4',
__typename: 'User',
name: 'Zuck',
},
},
});
const callback = jest.fn();
refetch(refetchVariables, null, callback);
expect(callback).toHaveBeenCalled();
});

it('returns false for isLoading if the response comes from cache', () => {
const refetchVariables = {
cond: false,
id: '4',
};
const fetchedVariables = {id: '4'};
environment.mock.cachePayload(UserQuery, fetchedVariables, {
data: {
node: {
id: '4',
__typename: 'User',
name: 'Zuck',
},
},
});
refetch(refetchVariables, null, jest.fn());
expect(environment.mock.isLoading(UserQuery, fetchedVariables)).toBe(
false,
);
});

it('renders with the results of the new variables on success', async () => {
expect.assertions(5);
expect(render.mock.calls.length).toBe(1);
Expand Down

0 comments on commit 2ffb5c6

Please sign in to comment.