remotestorage.js
remotestorage.js copied to clipboard
remotestorage
refactor cachinglayer.js
the cachinglayer.js has some really funny behavior with that weird callback and doUpdateNodes stuff. It first needs some documentation to describe it's behavior and why certain things are done (whoever implemented this should be in charge of that). Then after it's been documented and others can get their heads around it, it should be refactored to simplify it as much as possible.
Who here has worked with cachinglayer.js the most and can start on some documentation?
@galfert and I both have quite a good understanding of how it works, we made some changes to it a few times. IIRC it was originally implemented by @galfert and @skddc when they did 0.9 - but should be able to see that in the git history
cool, so who can take a half hour or so to try to document the behavior (and importantly it's interface with the rest of the code-base)?
Yes, my understanding of cachinglayer.js is probably better than for example sync.js :)
The original implementation I did was just consolidating some duplicated code from indexeddb.js, inmemorystorage.js and localstorage.js into a common file. After that it changed a lot with @michielbdejong's sync-per-node rewrite. There are definitely some parts that I don't fully understand yet, though. Mostly the whole update nodes and queueing behavior.
Yes, it's mostly @michielbdejong design and we tried to refactor it as best as we could during the review sessions for that monster PR. But in @galfert's and my opinion it has to be completely rewritten at some point. Your proposal of documenting it first makes a lot of sense. It took days to wrap our heads around this stuff last time.
I'll answer @silverbucket's questions first - The important thing to know about cachinglayer.js is that it's called in a funny way. It defines a bunch of methods and then in https://github.com/remotestorage/remotestorage.js/blob/master/src/cachinglayer.js#L401-L419 does the mix-in of those functions into your local store. The benefit of this is, as @galfert said, that these methods are shared by all local store implementations (indexeddb, localstorage, and inmemorystorage). This work was done by @galfert in December.
A lot of what these methods do interacts with what each local store defines, namely mostly storeNodes, getNodes, and forAllNodes. What updateNodes does is simple: it gets a bunch of nodes, allows you to change them in a callback, and saves them. It's a bit like a DAO, I guess.
In the last patch release 20 days ago we had to complicate cachinglayer.js a bit because of a race condition.
This code block makes sure updateNodes only runs once at a time. It does so by storing the promise for each time you call it, in a queue. It's quite similar to how the IndexedDB commit cache works and also how Sync queues get requests on tasks.
Then it lets your callback edit the content of the node. That's an important line. _processNodes is the callback you pass it, which does the actual work. I guess the name is a bit confusing next to _updateNodes and _doUpdateNodes. But that's really all there is to it.
- Queue requests by storing their promises, serve them one by one
- get nodes; update nodes; store nodes
HTH!
And to answer @skddc's comment: Yes, by all means, please completely rewrite this file! I think some of our code would indeed be much better if you would have written it. And the only person who can fix that is, by definition, you. :)
No need to be rude. There's no offense intended, I just reported that we had trouble getting our heads around it, and whenever someone wants to debug or improve it it's the same thing. That doesn't mean it shouldn't have been written or doesn't fulfill its purpose. A lot of parts of the library are just too hackish and chaotic for people to debug and contribute easily. If we're not honest about that, we cannot improve it.
I also realized that I was talking about sync.js mostly, because I mixed up the classes in my head. And mixing in common functions in the storage classes was a refactoring and makes total sense. As soon as we can use ES6 modules, we can do this in a non-hacky cleaner way.