Expose maxCacheSize opt (for system db) #156
Conversation
|
Move the checkout to be an optional arg also so we dont have optional positionals |
Done (I don't think SystemView is part of the public API, so should be fine, but in theory this is a breaking change) |
lib/system.js
Outdated
| @@ -68,7 +70,13 @@ module.exports = class SystemView extends ReadyResource { | |||
| } | |||
|
|
|||
| async checkout (length) { | |||
| const checkout = new SystemView(this.core.session(), length) | |||
| const checkout = new SystemView( | |||
There was a problem hiding this comment.
Note: this was added with the last commit (had missed this usage of new SystemView on my initial pass, so it wasn't yet passing through the maxCacheSize)
There was a problem hiding this comment.
what do you think about making the opt explicit here?
There was a problem hiding this comment.
checkout(length, { maxCacheSize = this.db.maxCacheSize } = {})
There was a problem hiding this comment.
Yeah, makes sense. See #156 (comment) though, we might be iterating on a method that's no longer relevant
There was a problem hiding this comment.
ok added it anyways
lib/system.js
Outdated
| @@ -12,12 +12,14 @@ const DIGEST = subs.sub(b4a.from([0])) | |||
| const MEMBERS = subs.sub(b4a.from([1])) | |||
|
|
|||
| module.exports = class SystemView extends ReadyResource { | |||
| constructor (core, checkout = 0) { | |||
| constructor (core, opts = {}) { | |||
There was a problem hiding this comment.
{ checkout = 0, maxCacheSize = 0 } = {}
lib/system.js
Outdated
| this.core.session(), | ||
| { | ||
| checkout: length, | ||
| maxCacheSize: this.maxCacheSize |
There was a problem hiding this comment.
dont think this prop is on the system, you should get it from the existing db
There was a problem hiding this comment.
You're right, well-spotted, I fixed it.
I think that method is never used though (from what I can see, it's never used in autobase itself, nor is it tested, and I don't think it makes sense to access it directly from outside autobase), so might make sense to just remove it in a follow-up PR, or perhaps add a test
There was a problem hiding this comment.
ahh I think checkout will stick around for a while, it's quite useful to have
chm-diederichs
force-pushed
the
max-cache-size-opt
branch
from
03c23b4 to
01c69b8
Compare
Note: the current implementation is not ideal for maintaining, given that the
SystemViewis created in 5 different places and it needs to be explicitly passed themaxCacheSizeoption every time. Might make sense to refactor to acreateSystemView(core, length)method which is called in each of those 5 cases, wherecreateSystemViewis responsible for adding the opts (for now onlymaxCacheSize).