Closed jgaehring closed 4 years ago
I'm going to have to rethink generateLogID
and how it is coupled with our database operations. This may be unavoidable, but I need to think through the possible side effects if we're not caching every log but still requiring every log have a localID
.
Took a first pass at this with jgaehring/farmOS-client@adf9590. I think it's pretty good, works as intended. I've got a unit test in place for it.
I don't think there's too much more to do, except I want to see how it plays out once I implement the new syncing API, just to make sure there's nothing I overlooked.
I'm going to have to rethink
generateLogID
and how it is coupled with our database operations. This may be unavoidable, but I need to think through the possible side effects if we're not caching every log but still requiring every log have alocalID
.
This could be avoided with the adoption of UUID's (#395).
Right now our caching strategy is pretty much "all or nothing"; everything that's created on the device or synced to the device is cached, so in many ways our syncing strategy is closely coupled to concerns for what and how much we want to cache. We can make this more flexible by imposing an explicit caching strategy, which can be as simple as "only cache logs from the last 30 days, the next 15 days, or which haven't been synced yet." Eventually we might want to make it more complex or enable Field Modules to directly effect what gets cached and what doesn't, but I think that will be a good start. See #367 for some more background on these issues.
In addition, I want to get rid of the caching metadata that we're currently storing on each log object, specifically the
isCachedLocally
property, but also themodules
property, which is mostly just there for the sake of caching.isCachedLocally
could be eliminated pretty easily, I think, if we just get rid of lines 28-32 in theupdateCachedLog
action:https://github.com/farmOS/farmOS-client/blob/20f8eca6e719f459b4d120ac7bed96d263ce9f20/src/core/store/idb/module.js#L25-L34
That
addLogs
mutation would trigger an infinite loop if we didn't have theisCachedLocally
prop, but those lines wouldn't even be necessary if it wasn't for the prop. So it would be much simpler that way.This should also aid in #343 by eliminating some of the potential for race conditions. For that reason, I might bump this up in priority, since that bug needs to be fixed ASAP.