-
Notifications
You must be signed in to change notification settings - Fork 3
smart-queue next iteration WIP #5
Conversation
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.
Hi @echoes221 ,
The way you have structured the code base it's an amazing work man.
96e1335
to
3076a7f
Compare
@prsn thanks man :) Glad you like 👍 |
Thanks for the great work I haven't had the chance to take a look into this carefully. I will try tomorrow! |
3cf5780
to
25862ff
Compare
Updated majority of tests, only one missing now is for the merge strategy. |
[ | ||
'it removes CREATE outbox action when incoming DELETE action has the same key from outbox', | ||
createAction(DELETE, 1), | ||
[read1, update1, delete1] |
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 the delete1 shouldn't be in the outbox. If we remove the create1 there's no point on performing a delete1 on our backend
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.
It's a little confusing, delete1 is Id 4 as per the constant setup, the delete coming in is ID 1. It's making sure that we don't override another delete. Maybe it's worth updating the constants to reflect their ID?
Ok looks amazing great job!! I wonder have you thought about what to do with the merge function |
@sorodrigo I'm glad you like! The type: export type QueueFunction = {
(Outbox, OfflineAction, number, Config): Outbox,
allowedMethods: Method[]
}; Using that is a bit awkward on a queue function: // This errors
export const readInQueue: QueueFunction = (outbox, action, index, { merge }) =>
replaceAtIndex(outbox, merge(outbox[index], action), index);
readInQueue.allowedMethods = [READ]; Errors because I've had some difficulty with flow and my IDE, it's a little clunky and slow compared to my (limited) experience with TS. I would suggest to move to TS to prevent us needing to manually type the Regarding the merge strategy any help would be appreciated. I've left a couple of TODO's on the function itself. It's more about handling the optional |
So I settled on the Object.assign method for typing the callable objects. Not as neat, but it works. Just the merging strategy to solidify. I'll have a think on it to see if I can come up with a smart way of doing it. |
} | ||
|
||
const queueFunction = getQueueFunction(queueMeta); | ||
const index = indexOfAction(outbox, queueMeta); |
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.
indexOfAction
should also contain the check for methodAllowed
as certain actions cannot be squashed over each other. We should aim to find the best available, not give up on the first id match which may not allow a merge strategy to take place. E.g. enqueue([Read, Update], Update) -> [Read, Update, Update]
, first check fails so we add to the queue as new entity, where what we actually want is [Read, Update]
WORK IN PROGRESS
Wanted to ping this up to get some feedback on the direction that this is going (adapting @prsn implementation) and continue to work on with feedback. I've not completely tested this yet so YMMV.
Addressing redux-offline/queue#4 And #4
Changes:
TODO:
TestsTypes/Flowlodash.get
to simplify this) and body/json types on effect