-
Notifications
You must be signed in to change notification settings - Fork 270
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
Conversation
Clean implementation 👌 I personally would prefer naming this API in a simpler way, abstracting away the pattern that is used (semaphore), i.e. having actions called e.g. However, how would you compare this imperative "ON/OFF" API vs. something more declarative like I did here? It allows me to just specify one single state selector that can enable/disable the queue automatically, as it is just a function of the current application state. Typically it works like this: const networkMiddleware = createNetworkMiddleware({
shouldDequeueSelector: state => state.session.isAuthenticated,
...
}) Anyway, this looks like a nice & useful addition to the library! |
Hey, thanks for the feedback! I did think about the naming, more than happy to rename. How about Regarding the API, having a selector in the middleware config was indeed something we thought about, yet we didn't see this implementation. I think it's cleaner, thanks for the tip! Question, in your code I don't see any further check in |
Alright, I've implemented it in a cleaner way following @melvynhills suggestion. I've opened a PR on our fork as I'm short on time to fix unit tests. If people want to take a look, here's our implementation with a deselector. https://github.com/evrythng/react-native-offline/pull/2/files. Thanks @melvynhills for the tip! |
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.
Nice work, thanks for this! 🎉
In terms of the implementation, I'd suggest a couple of changes:
1.- Move the flag hasQueueBeenHalted
into createNetworkMiddleware
, inside the closure, instead of a new reducer piece of state. I feel this is something internal to the library, so it fits better if it's not publicly exposed to the consumer. Furthermore, I wouldn't expect that driving any UI state.
Something along these lines:
function createNetworkMiddleware({
regexActionType = /FETCH.*REQUEST/,
actionTypes = [],
queueReleaseThrottle = 50,
}: Arguments = {}) {
let hasQueueBeenHalted = false; // We keep it in the closure
return ({ getState }: MiddlewareAPI<State>) => (
next: (action: any) => void,
) => (action: any) => {
const { isConnected, actionQueue } = getState().network;
const releaseQueue = createReleaseQueue(
getState,
next,
queueReleaseThrottle,
);
...
Then the new action would just mutate that variable and the queue would operate based upon it.
2.- Rename the action creator as per my comments. Also, maybe change the payload to an enum (object) we could export, something like Semaphore.RED
, Semaphore.GREEN
, to be more descriptive. Passing a boolean doesn't reflect perfectly the intent. Also, we could extend it in the future for other use cases.
Looking forward to hearing from you :)
@rgommezz I think @emanueleDiVizio's plan is to do it in a more declarative way, as most people seem to like the idea of a state selector: https://github.com/evrythng/react-native-offline/pull/2/files |
Oh cool @melvynhills, sorry I missed the last comment and the separate fork, went straight to the code. I like that approach as well and would definitely be up to get this merged, could we make this happen @emanueleDiVizio? I can help with the tests if needed, just point your changes to this repo. |
So quick update, I've ended up using both mechanisms in our codebase, as I couldn't find a way to stop the queue with the selector if it's already been released. I couldn't manage to update this PR yet, so code is in my fork for now. If you guys are happy to have both approaches available, I'll be glad to clean up code and update this PR. I'll also address @rgommezz comments 👍 To speed this up, @rgommezz (and everyone else who's up for it) would you mind checking out emanueleDiVizio@59244e6 to see if anything could be improved? I'll then open a PR here asap. |
Okay great, I've managed to have both approaches side by side. In the last two commits, I've implemented community suggestions and refactored a little bit. We also have tests for new functionality. Think I've ended up having both community suggestions and state selector into a single commit, apologies. Next thing I think would be to update the docs. @rgommezz I've had a thought about moving the queue flag out of the reducer, yet I didn't manage to do it just yet. The point is that If anyone else (or me later on) manages to move out the Let me know if there's any other way I can help! Edit: I ended up trying @rgommezz approach, that is using an inner variable instead of using the store. The problem of this approach is that a new If anyone has any idea on how to improve this, I'd be more than happy to listen! |
Update, after last push |
Alright, I've fixed it, tests are now passing. Semaphore is working and deselector is working. Only thing is if queue deselctor changes from true to false, the queue is not automatically released. Apart from this, doc update is last thing left I reckon. @rgommezz If everything looks good, I can provide docs 👍 |
Btw @rgommezz, I can't fathom why but I have been using a DAZN pen for the past few days :') I've found it in my office... |
I am happy to test it if you can give me some pointers on how to use it. I am using redux-saga |
So we have two ways to control the queue. One is with |
I'll try it out this week if I get some time. When I've tested it, I'll get back to you! |
@emanueleDiVizio I briefly glanced at it during the weekend and it look quite solid, well done! 🎉 I'll fully re-review tomorrow |
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.
Left some comments, but it's almost ready!
Please revert the yarn.lock
addition. This repo uses npm
src/redux/createNetworkMiddleware.js
Outdated
function didQueueResume(action, isQueuePaused) { | ||
return ( | ||
action.type === networkActionTypes.CHANGE_QUEUE_SEMAPHORE && | ||
!isQueuePaused && |
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.
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'
);
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 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?
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.
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
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.
Ah, I see! yes good catch, I'll fix this 👍
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.
Done 👍
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.
Can confirm fix is in d0393a6
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.
Alright done, do we also want to export SEMAPHORE_COLOR
const?
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.
You changed a different function on d0393a6 😕
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.
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
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.
Uh, my bad! Will fix it 👍
if (isBackOnline) { | ||
const hasQueueBeenResumed = didQueueResume(action, isQueuePaused); | ||
|
||
const shouldDequeue = |
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.
What about this regrouping, since isBackOnline
by itself was already a valid condition?
const shouldDequeue =
(isBackOnline || (isConnected && hasQueueBeenResumed)) &&
shouldDequeueSelector(getState());
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.
Good idea, I'll implement this 👍
}: Arguments = {}) { | ||
return ({ getState }: MiddlewareAPI<State>) => ( | ||
next: (action: any) => void, | ||
) => (action: any) => { | ||
const { isConnected, actionQueue } = getState().network; | ||
const { isConnected, actionQueue, isQueuePaused } = getState().network; |
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.
Tiny opt. Let's keep the state in a variable right away since we are calling getState()
twice.
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.
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?
@@ -4,6 +4,8 @@ export type State = { | |||
isConnected: boolean, | |||
}; | |||
|
|||
export type SemaphoreColor = 'RED' | 'GREEN'; |
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.
💯
Done, I've removed the |
Alright good news, I've also updated docs. Let me know if anything can be improved there! |
…ative-offline into feature/queue-semaphore
Btw @emanueleDiVizio, I can't merge this without having 100% coverage to make sure there are no regressions. You can take a look by running It seems that function ( |
Sure thing, I'll clean up and address tests between today and tomorrow 👍 |
Alright good news, I've fixed |
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.
Nice, thanks for the changes.
I've added some last suggestions for polishing (you can commit them directly from GitHub) and a couple of comments regarding tests.
This is pretty much ready to be shipped 👏
README.md
Outdated
@@ -347,6 +347,7 @@ type MiddlewareConfig = { | |||
regexActionType?: RegExp = /FETCH.*REQUEST/, | |||
actionTypes?: Array<string> = [], | |||
queueReleaseThrottle?: number = 50, | |||
shouldDequeueSelector: (state: State) => boolean = state => state.conditionToReleaseQueue |
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.
shouldDequeueSelector: (state: State) => boolean = state => state.conditionToReleaseQueue | |
shouldDequeueSelector: (state: RootReduxState) => boolean = () => true |
README.md
Outdated
@@ -360,6 +361,9 @@ By default it's configured to intercept actions for fetching data following the | |||
|
|||
`queueReleaseThrottle`: waiting time in ms between dispatches when flushing the offline queue. Useful to reduce the server pressure when coming back online. Defaults to 50ms. | |||
|
|||
`shouldDequeueSelector`: state selector used to control if the queue should be released on connection change. Returning `true` releases the queue, returning `false` prevents queue release. Note, if the result of `shouldDequeueSelector` changes *while* the queue is being released, the queue will not halt. If you want to halt the queue *while* is being released, please see relevant FAQ section. |
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.
`shouldDequeueSelector`: state selector used to control if the queue should be released on connection change. Returning `true` releases the queue, returning `false` prevents queue release. Note, if the result of `shouldDequeueSelector` changes *while* the queue is being released, the queue will not halt. If you want to halt the queue *while* is being released, please see relevant FAQ section. | |
`shouldDequeueSelector`: function that receives the redux application state and returns a boolean. It'll be executed every time an action is dispatched, before it reaches the reducer. This is useful to control if the queue should be released when the connection is regained and there were actions queued up. Returning `true` (the default behaviour) releases the queue, whereas returning `false` prevents queue release. For example, you may wanna perform some authentication checks, prior to releasing the queue. Note, if the result of `shouldDequeueSelector` changes *while* the queue is being released, the queue will not halt. If you want to halt the queue *while* is being released, please see relevant FAQ section. |
@@ -8,11 +8,13 @@ import type { | |||
FluxActionWithPreviousIntent, | |||
FluxActionForRemoval, | |||
NetworkState, | |||
SemaphoreColor, | |||
} from '../types'; | |||
|
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.
import { SEMAPHORE_COLOR } from '../utils/constants'; |
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.
Have to keep SemaphoreColor
for typing in handleChangeQueueSemaphore
(line 88). I'll import SEMAPHORE_COLOR
as well.
src/redux/createReducer.js
Outdated
): NetworkState { | ||
return { | ||
...state, | ||
isQueuePaused: semaphoreColor === 'RED', |
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.
isQueuePaused: semaphoreColor === 'RED', | |
isQueuePaused: semaphoreColor === SEMAPHORE_COLOR.RED, |
test/createNetworkMiddleware.test.js
Outdated
expect(mockDispatch).toHaveBeenNthCalledWith(4, 'bar'); | ||
}); | ||
|
||
it('does not empty the queue if we are online and dequeue selector returns false', async () => { |
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.
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);
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.
Yeah that's the correct implementation, I'll address this 👍
test/reducer.test.js
Outdated
@@ -383,3 +406,11 @@ describe('networkSelector', () => { | |||
}); | |||
}); | |||
}); | |||
|
|||
describe('network reducer config', () => { |
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.
What's the purpose of this test?
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 think this is a leftover from previous code, I'll remove it. Good catch 👍
I've addressed comments 👍 |
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.
Thanks for the patience and great work @emanueleDiVizio! This is ready to be shipped. I'll merge it later on and will publish a new version on npm. Don't worry about that test case scenario for now, it's good we are back at 100% coverage! :D
Yuppie! Thanks for the great support @rgommezz! |
@all-contributors please add @emanueleDiVizio for code and doc |
I've put up a pull request to add @emanueleDiVizio! 🎉 |
Nice work guys! Unfortunately I wasn't able to test it yet, I'll try to get to it ASAP, as we also need this in our app. Will provide feedback when it got tested from my side. |
Motivation
While using this library, we encountered the need for halting the queue and resume it based on different conditions. In our app, we have two use cases for this. Since we see other people having the same issue (#160), we thought to write a PR.
First, wait for the user to authenticate for the queue to be released. Second, we treat certain logic as blocking, so the queue will wait for the logic to be complete before resume.
This is implemented with a simple semaphore mechanism.
Test plan
The idea behind this PR is to have a semaphore mechanism for the queue. The logic is very similar to the one used to handle connectivity change.
We have a semaphore variable
hasQueueBeenHalted
.To toggle the semaphore we've implemented the
queueSemaphoreChange(shouldHaltQueue: boolean)
action. IfshouldHaltQueue
is set totrue
, thenhasQueueBeenHalted
will be set to true and the queue will stop being released.If
shouldHaltQueue
is set to false, thenhasQueueBeenHalted
will be set tofalse
and the queue will be resumed. This is all done only if eitherisConnected
is true or it has been recently toggled.I've added a couple of tests for the functionality.
Note, this is a working MVP at the moment. I'm pushing a PR already to gain early feedback and to let fellow developers benefit from this. Do please comment on this and share how it can be improved.
Closes #160