Extend MemoryStore class with synced properties #7
Conversation
| * getSynced, setSynced. | ||
| * | ||
| * @param {object} [options] Store options. | ||
| * @param {number} [options.synced=0] Events with lower `added` time in current |
There was a problem hiding this comment.
If you was confused about names first time, maybe we should change key names? current/other?
There was a problem hiding this comment.
As they will be properties of store object – just current/other will be not enough. How about currentSynced/otherSynced?
There was a problem hiding this comment.
Looks good. But current not looks so clear. Sync#currentProtocol looks like a protocol version of current connection. Also other prefix is not good too.
Maybe there are a special terms for it in other protocol docs?
There was a problem hiding this comment.
In couple swift libraries I found local/remote prefixes for this case. Also in scuttlebut (https://github.com/dominictarr/scuttlebutt/blob/master/index.js) they used latest + syncSent/syncRecv. I thinking about something like latestClient/latestServer or clientSynced/serverSynced as it will be used in client nodes – so we'll always know what is client and what is server in this case. Also yeah – can work localSynced/remoteSynced
There was a problem hiding this comment.
Great overview. I like the idea of localProtocol/remoteProtocol!
But cleint* and server* prefix could not be used, because logux-sync could be used in server-server communication :(. But latestSent, latestReceived looks great! Maybe even better than localSynced/remoteSynced, because it is more clear.
There was a problem hiding this comment.
setOptions and setExtra don't tell anything about synchronization state, looks too general, like if you want to set name of store or something. Maybe setLatestSync?
There was a problem hiding this comment.
Yeap, setLatestSynced({ sent, received }) looks good.
But maybe having a general open-structure data is good too? What if some future Logux plugins will need to save some data to Store?
There was a problem hiding this comment.
Yes, I think it can be useful (setOptions/setProperties/setData/setExtra) in future but as we don't have cases now – lets add this *LatestSynced methods and after we'll implement more database stores – we'll see what we could need and then change API. I think it will be clear before next version release and we'll change it to more general names later if we'll need it
There was a problem hiding this comment.
Updated code and tests to *LatestSynced with argument and return value as parameter
| this.created = [] | ||
| this.added = [] | ||
| this.options = options || {} | ||
| this.synced = this.options.synced || 0 |
There was a problem hiding this comment.
Why do we need this options if we can’t set default events in MemorySrore?
There was a problem hiding this comment.
Yeah, it is redundant here.
| */ | ||
| getSynced: function getSynced () { | ||
| var store = this | ||
| return new Promise(function (resolve) { |
There was a problem hiding this comment.
There is Promise.resolve(value) shortcut for it. Same with setSynced.
@ai What do you think about API for
synced/otherSyncedlike this? Please check if it is ok – I'll write tests