Skip to content
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

Implement control mechanism for queue release #225

Merged
merged 18 commits into from
Oct 20, 2019
Merged
Show file tree
Hide file tree
Changes from 10 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: 1 addition & 1 deletion src/components/NetworkConnectivity.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ function validateProps(props: Props) {
throw new Error('httpMethod parameter should be either HEAD or OPTIONS');
}
}

/* eslint-disable react/default-props-match-prop-types */
class NetworkConnectivity extends React.PureComponent<Props, State> {
static defaultProps = {
onConnectivityChange: () => undefined,
Expand Down
3 changes: 3 additions & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ module.exports = {
get offlineActionTypes() {
return require('./redux/actionTypes').default;
},
get offlineActionCreators() {
return require('./redux/actionCreators').default;
},
get networkSaga() {
return require('./redux/sagas').default;
},
Expand Down
17 changes: 17 additions & 0 deletions src/redux/actionCreators.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import type {
FluxActionWithPreviousIntent,
FluxActionForRemoval,
FluxActionForDismissal,
FluxActionForChangeQueueSemaphore,
SemaphoreColor,
} from '../types';

type EnqueuedAction = FluxAction | Function;
Expand Down Expand Up @@ -53,3 +55,18 @@ export const dismissActionsFromQueue = (
type: actionTypes.DISMISS_ACTIONS_FROM_QUEUE,
payload: actionTrigger,
});

export const changeQueueSemaphore = (
semaphoreColor: SemaphoreColor,
): FluxActionForChangeQueueSemaphore => ({
type: actionTypes.CHANGE_QUEUE_SEMAPHORE,
payload: semaphoreColor,
});

export default {
emanueleDiVizio marked this conversation as resolved.
Show resolved Hide resolved
changeQueueSemaphore,
dismissActionsFromQueue,
removeActionFromQueue,
fetchOfflineMode,
connectionChange,
};
2 changes: 2 additions & 0 deletions src/redux/actionTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ type ActionTypes = {|
FETCH_OFFLINE_MODE: '@@network-connectivity/FETCH_OFFLINE_MODE',
REMOVE_FROM_ACTION_QUEUE: '@@network-connectivity/REMOVE_FROM_ACTION_QUEUE',
DISMISS_ACTIONS_FROM_QUEUE: '@@network-connectivity/DISMISS_ACTIONS_FROM_QUEUE',
CHANGE_QUEUE_SEMAPHORE: '@@network-connectivity/CHANGE_QUEUE_SEMAPHORE',
|};

const actionTypes: ActionTypes = {
Expand All @@ -13,6 +14,7 @@ const actionTypes: ActionTypes = {
REMOVE_FROM_ACTION_QUEUE: '@@network-connectivity/REMOVE_FROM_ACTION_QUEUE',
DISMISS_ACTIONS_FROM_QUEUE:
'@@network-connectivity/DISMISS_ACTIONS_FROM_QUEUE',
CHANGE_QUEUE_SEMAPHORE: '@@network-connectivity/CHANGE_QUEUE_SEMAPHORE',
};

export default actionTypes;
25 changes: 21 additions & 4 deletions src/redux/createNetworkMiddleware.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ type Arguments = {|
regexActionType: RegExp,
actionTypes: Array<string>,
queueReleaseThrottle: number,
shouldDequeueSelector: (state: State) => boolean,
|};

function validateParams(regexActionType, actionTypes) {
Expand Down Expand Up @@ -70,11 +71,19 @@ function didComeBackOnline(action, wasConnected) {
);
}

function didQueueResume(action, isQueuePaused) {
return (
action.type === networkActionTypes.CHANGE_QUEUE_SEMAPHORE &&
!isQueuePaused &&
Copy link
Owner

Choose a reason for hiding this comment

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

Wouldn't this entail that the queue was paused before?

So the condition would be:

  return (
    action.type === networkActionTypes.CHANGE_QUEUE_SEMAPHORE &&
    isQueuePaused && // shouldn't it be like this?
    action.payload === 'GREEN'
  );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe we're checking that the queue was paused before this action is received, since by the time the middleware receives CHANGE_QUEUE_SEMAPHORE, isQueuePaused hasn't been updated yet. So I think that !isQueuePaused is the correct check. Does that sound right?

Copy link
Owner

@rgommezz rgommezz Oct 16, 2019

Choose a reason for hiding this comment

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

As you are saying, we're checking that the queue was paused before this action is received, so isQueuePaused === true at the moment the action arrives to the middleware. The semaphore action indicates that we should unblock/unpause the queue.

So for me, didQueueResume means that:

  • And action of type action.type === networkActionTypes.CHANGE_QUEUE_SEMAPHORE is about to be dispatched.
  • The queue is currently paused: isQueuePaused === true
  • The payload is SemaphoreColor.GREEN, meaning we should unpause/unblock the queue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see! yes good catch, I'll fix this 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can confirm fix is in d0393a6

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright done, do we also want to export SEMAPHORE_COLOR const?

Copy link
Owner

Choose a reason for hiding this comment

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

You changed a different function on d0393a6 😕

Copy link
Owner

Choose a reason for hiding this comment

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

True, I've thought about it. Not sure how it plays with Flow tho, since we've defined enums for it... Should I just add a SemaphoreColors object without flow?

Yes, I think that's better. Once we migrate to Typescript in the future, we'll be able to leverage Enums

Copy link
Contributor Author

@emanueleDiVizio emanueleDiVizio Oct 17, 2019

Choose a reason for hiding this comment

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

Uh, my bad! Will fix it 👍

action.payload === 'GREEN'
);
}

export const createReleaseQueue = (getState, next, delay) => async queue => {
// eslint-disable-next-line
for (const action of queue) {
const { isConnected } = getState().network;
if (isConnected) {
const { isConnected, isQueuePaused } = getState().network;
if (isConnected && !isQueuePaused) {
next(removeActionFromQueue(action));
next(action);
// eslint-disable-next-line
Expand All @@ -89,11 +98,12 @@ function createNetworkMiddleware({
regexActionType = /FETCH.*REQUEST/,
actionTypes = [],
queueReleaseThrottle = 50,
shouldDequeueSelector = () => true,
}: Arguments = {}) {
return ({ getState }: MiddlewareAPI<State>) => (
next: (action: any) => void,
) => (action: any) => {
const { isConnected, actionQueue } = getState().network;
const { isConnected, actionQueue, isQueuePaused } = getState().network;
Copy link
Owner

Choose a reason for hiding this comment

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

Tiny opt. Let's keep the state in a variable right away since we are calling getState() twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the reason for calling getState inside the releaseQueue loop is that this way the queue release function always has the latest state on every iteration. This way, when CHANGE_QUEUE_SEMAPHORE and CONNECTION_CHANGE are dispatched the new state is reflected straight away inside releaseQueue, since at every iteration we call getState(). On the other hand, if we pass down isQueuePaused and isConnected from middleware function their values will only reflect the state when the releaseQueue was created, and will not update on further state change while the queue is being released. Does that sound right?

const releaseQueue = createReleaseQueue(
getState,
next,
Expand All @@ -114,7 +124,14 @@ function createNetworkMiddleware({
}

const isBackOnline = didComeBackOnline(action, isConnected);
if (isBackOnline) {
const hasQueueBeenResumed = didQueueResume(action, isQueuePaused);

const shouldDequeue =
Copy link
Owner

Choose a reason for hiding this comment

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

What about this regrouping, since isBackOnline by itself was already a valid condition?

    const shouldDequeue =
      (isBackOnline || (isConnected && hasQueueBeenResumed)) &&
      shouldDequeueSelector(getState());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I'll implement this 👍

isConnected &&
(isBackOnline || hasQueueBeenResumed) &&
shouldDequeueSelector(getState());

if (shouldDequeue) {
// Dispatching queued actions in order of arrival (if we have any)
next(action);
return releaseQueue(actionQueue);
Expand Down
18 changes: 17 additions & 1 deletion src/redux/createReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,13 @@ import type {
FluxActionWithPreviousIntent,
FluxActionForRemoval,
NetworkState,
SemaphoreColor,
} from '../types';

Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
import { SEMAPHORE_COLOR } from '../utils/constants';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have to keep SemaphoreColor for typing in handleChangeQueueSemaphore (line 88). I'll import SEMAPHORE_COLOR as well.

export const initialState = {
isConnected: true,
actionQueue: [],
isQueuePaused: false,
};

function handleOfflineAction(
Expand Down Expand Up @@ -81,8 +83,20 @@ function handleDismissActionsFromQueue(
};
}

function handleChangeQueueSemaphore(
state: NetworkState,
semaphoreColor: SemaphoreColor,
): NetworkState {
return {
...state,
isQueuePaused: semaphoreColor === 'RED',
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
isQueuePaused: semaphoreColor === 'RED',
isQueuePaused: semaphoreColor === SEMAPHORE_COLOR.RED,

};
}

export default (comparisonFn: Function = getSimilarActionInQueue) => (
state: NetworkState = initialState,
state: NetworkState = {
...initialState,
},
action: *,
): NetworkState => {
switch (action.type) {
Expand All @@ -97,6 +111,8 @@ export default (comparisonFn: Function = getSimilarActionInQueue) => (
return handleRemoveActionFromQueue(state, action.payload);
case actionTypes.DISMISS_ACTIONS_FROM_QUEUE:
return handleDismissActionsFromQueue(state, action.payload);
case actionTypes.CHANGE_QUEUE_SEMAPHORE:
return handleChangeQueueSemaphore(state, action.payload);
default:
return state;
}
Expand Down
8 changes: 8 additions & 0 deletions src/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ export type State = {
isConnected: boolean,
};

export type SemaphoreColor = 'RED' | 'GREEN';
Copy link
Owner

Choose a reason for hiding this comment

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

💯


export type FluxAction = {
type: string,
payload: any,
Expand Down Expand Up @@ -35,9 +37,15 @@ export type FluxActionForDismissal = {
payload: string,
};

export type FluxActionForChangeQueueSemaphore = {
type: string,
payload: SemaphoreColor,
};

export type NetworkState = {
isConnected: boolean,
actionQueue: Array<*>,
isQueuePaused: boolean,
};

export type HTTPMethod = 'HEAD' | 'OPTIONS';
55 changes: 50 additions & 5 deletions test/createNetworkMiddleware.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import createNetworkMiddleware, {
createReleaseQueue,
} from '../src/redux/createNetworkMiddleware';
import * as actionCreators from '../src/redux/actionCreators';
import { removeActionFromQueue } from '../src/redux/actionCreators';
import wait from '../src/utils/wait';

const getFetchAction = type => ({
Expand Down Expand Up @@ -378,14 +377,16 @@ describe('createReleaseQueue', () => {
const mockGetState = jest.fn().mockImplementation(() => ({
network: {
isConnected: true,
isQueuePaused: false,
},
}));
const mockDelay = 50;
afterEach(() => {
mockDispatch.mockClear();
mockGetState.mockClear();
});
it('empties the queue if we are online', async () => {

it('empties the queue if we are online and queue is not halted', async () => {
const releaseQueue = createReleaseQueue(
mockGetState,
mockDispatch,
Expand All @@ -396,12 +397,35 @@ describe('createReleaseQueue', () => {
expect(mockDispatch).toHaveBeenCalledTimes(4);
expect(mockDispatch).toHaveBeenNthCalledWith(
1,
removeActionFromQueue('foo'),
actionCreators.removeActionFromQueue('foo'),
);
expect(mockDispatch).toHaveBeenNthCalledWith(2, 'foo');
expect(mockDispatch).toHaveBeenNthCalledWith(
3,
removeActionFromQueue('bar'),
actionCreators.removeActionFromQueue('bar'),
);
expect(mockDispatch).toHaveBeenNthCalledWith(4, 'bar');
});

it('does not empty the queue if we are online and dequeue selector returns false', async () => {
Copy link
Owner

Choose a reason for hiding this comment

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

Help me understand this test, if the dequeue selector returns false, the queue is not emptied. However, I can see actions dispatched about removing stuff from the queue.

I think our best shot here is to pass the dequeueSelector fn to createReleaseQueue as the last argument (as you are already presuming in this test, but the implementation doesn't have it).

Then basically the test becomes more reasonable:

await releaseQueue(actionQueue);
expect(mockDispatch).toHaveBeenCalledTimes(0);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's the correct implementation, I'll address this 👍

const mockDequeueSelector = () => false;
const releaseQueue = createReleaseQueue(
mockGetState,
mockDispatch,
mockDelay,
mockDequeueSelector,
);
const actionQueue = ['foo', 'bar'];
await releaseQueue(actionQueue);
expect(mockDispatch).toHaveBeenCalledTimes(4);
expect(mockDispatch).toHaveBeenNthCalledWith(
1,
actionCreators.removeActionFromQueue('foo'),
);
expect(mockDispatch).toHaveBeenNthCalledWith(2, 'foo');
expect(mockDispatch).toHaveBeenNthCalledWith(
3,
actionCreators.removeActionFromQueue('bar'),
);
expect(mockDispatch).toHaveBeenNthCalledWith(4, 'bar');
});
Expand All @@ -427,10 +451,31 @@ describe('createReleaseQueue', () => {
expect(mockDispatch).toHaveBeenCalledTimes(2);
expect(mockDispatch).toHaveBeenNthCalledWith(
1,
removeActionFromQueue('foo'),
actionCreators.removeActionFromQueue('foo'),
);
expect(mockDispatch).toHaveBeenNthCalledWith(2, 'foo');
});

it('should stop dispatching if queue has been halted', async () => {
const haltQueue = () =>
new Promise(async resolve => {
await wait(30);
mockGetState.mockImplementation(() => ({
network: {
isQueuePaused: true,
},
}));
resolve();
});
const releaseQueue = createReleaseQueue(
mockGetState,
mockDispatch,
mockDelay,
);
const actionQueue = ['foo', 'bar'];
await Promise.all([releaseQueue(actionQueue), haltQueue()]);
expect(mockDispatch).toHaveBeenCalledTimes(0);
});
});

describe('createNetworkMiddleware with wrong type params', () => {
Expand Down
31 changes: 31 additions & 0 deletions test/reducer.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const networkReducer = createReducer();
const getState = (isConnected = false, ...actionQueue) => ({
isConnected,
actionQueue,
isQueuePaused: false,
});

/** Actions used from now on to test different scenarios */
Expand Down Expand Up @@ -63,6 +64,7 @@ describe('CONNECTION_CHANGE action type', () => {
expect(networkReducer(initialState, mockAction)).toEqual({
isConnected: false,
actionQueue: [],
isQueuePaused: false,
});
});
});
Expand Down Expand Up @@ -106,6 +108,7 @@ describe('OFFLINE_ACTION action type', () => {
expect(nextState).toEqual({
isConnected: false,
actionQueue: [prevActionToRetry1],
isQueuePaused: false,
});

const action2 = actionCreators.fetchOfflineMode(prevActionToRetry2);
Expand Down Expand Up @@ -236,6 +239,26 @@ describe('REMOVE_ACTION_FROM_QUEUE action type', () => {
});
});

describe('QUEUE_SEMAPHORE_CHANGE action type', () => {
it('Pauses the queue if semaphore is red', () => {
expect(
networkReducer(undefined, actionCreators.changeQueueSemaphore('RED')),
).toEqual({
...initialState,
isQueuePaused: true,
});
});

it('Resumes the queue if semaphore is green', () => {
expect(
networkReducer(undefined, actionCreators.changeQueueSemaphore('GREEN')),
).toEqual({
...initialState,
isQueuePaused: false,
});
});
});

describe('thunks', () => {
function fetchThunk(dispatch) {
dispatch({ type: 'FETCH_DATA_REQUEST' });
Expand Down Expand Up @@ -383,3 +406,11 @@ describe('networkSelector', () => {
});
});
});

describe('network reducer config', () => {
Copy link
Owner

Choose a reason for hiding this comment

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

What's the purpose of this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is a leftover from previous code, I'll remove it. Good catch 👍

it('has isQueuePaused set to false by default', () => {
expect(networkReducer(undefined, { type: 'ACTION_I_DONT_CARE' })).toEqual(
initialState,
);
});
});
Loading