redux-offline / tools

MIT License
5 stars 3 forks source link

smart-queue next iteration WIP #5

Closed calumpeak closed 4 years ago

calumpeak commented 5 years ago

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 https://github.com/redux-offline/queue/issues/4 And #4

Changes:

TODO:

calumpeak commented 5 years ago

@prsn thanks man :) Glad you like 👍

sorodrigo commented 5 years ago

Thanks for the great work I haven't had the chance to take a look into this carefully. I will try tomorrow!

calumpeak commented 5 years ago

Updated majority of tests, only one missing now is for the merge strategy.

sorodrigo commented 5 years ago

Ok looks amazing great job!! I wonder have you thought about what to do with the merge function TODOs? Maybe I can help you test the remaining fn, I just need to know how to handle the TODOs.

calumpeak commented 5 years ago

@sorodrigo I'm glad you like! I've got most of the types done, I'm just having some difficulty with with the callableObject type and how to reference that correctly - I might have to make the QueueFuncs IIFE to type them correctly.

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 allowedMethods is not defined at that point. So I can either extend the function (Object.assign(func, { allowedMethods }), or wrap them both in an IIFE. Unless you've got a better insight into this?

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 t.ds file also, but redux-offline core is in flow so I'll save that for another conversation/next version (especially since babel7 has TS support baked in) :D

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 meta.offline keys, as well as providing support for effect.body and effect.json I thought about using lodash.get as that would allow us to statically type paths, and provide defaults on the fly without tonnes of conditionals.

calumpeak commented 5 years ago

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.