added onlyIfChanged opt #98
Conversation
rafapaezbas
force-pushed
the
only-if-changed-opt
branch
2 times, most recently
from
965a159 to
dbf5c5b
Compare
| const release = this.batchLock ? await this.batchLock() : null | ||
|
|
||
| const cas = (opts && opts.cas) || null | ||
| const cas = (opts && opts.cas) || (this.tree.onlyIfChanged ? sameData : null) |
There was a problem hiding this comment.
That's a tricky one in terms of API. What's the intended behaviour if a user specified onlyIfChanged: true, but also passes an explicit cas?
ATM it will ignore onlyIfChanged if the cas is passed. The alternative would be to run both the cas and the onlyIfChanged, and to proceed only if both pass. Not saying that is better, but it's important to explicitly decide what the behaviour should be.
Also in terms of API: I would also allow passing onlyIfChanged as an option to put and del
There was a problem hiding this comment.
Yeah, I think the intended behavior is that cas overwrites onlyIfChanged.
I think OnlyIfChanged option for put sounds like a good idea, for del probably it doesnt make sense since the default behavior is the same.
There was a problem hiding this comment.
There was a problem hiding this comment.
API is ok because method option overrides constructor option, just like with Hypercore timeout options, etc
test/basic.js
Outdated
| } | ||
| }) | ||
|
|
||
| test('onlyIfChanged del', async function (t) { |
There was a problem hiding this comment.
Not sure what's going on here, but it seems as if the cas never gets called on a delete, also when it's specified. Is it possible that a delete just does not run when there's nothing to delete?
(you can verify by setting onlyIfChanged to false, the test will still pass)
There was a problem hiding this comment.
yes, you are right, the default onlyInChanged delete cas (prev) => prev.value !== null, does not affect the behavior of the del operations at all. That means onlyIfChanged affects only put operations. I removed the useless logic from the PR.
rafapaezbas
force-pushed
the
only-if-changed-opt
branch
from
dbf5c5b to
5d726ea
Compare
| { | ||
| keyEncoding: 'binary', // "binary" (default), "utf-8", "ascii", "json", or an abstract-encoding | ||
| valueEncoding: 'binary' // Same options as keyEncoding like "json", etc | ||
| onlyIfChanged: true // put a value only if it means a change in the db |
There was a problem hiding this comment.
Alternative name: changeOnly
There was a problem hiding this comment.
I like the boolean option, but just maybe a different API could be a cas option in the constructor, so user can pass its global comparator
| { | ||
| keyEncoding: 'binary', // "binary" (default), "utf-8", "ascii", "json", or an abstract-encoding | ||
| valueEncoding: 'binary' // Same options as keyEncoding like "json", etc | ||
| onlyIfChanged: true // put a value only if it means a change in the db |
There was a problem hiding this comment.
Should say false because that's the actual default
| "protocol-buffers-encodings": "^1.2.0", | ||
| "ready-resource": "^1.0.0", | ||
| "safety-catch": "^1.0.2", | ||
| "same-data": "^1.0.0", |
There was a problem hiding this comment.
In which case would same-data not be enough? There is also same-object (what brittle uses)
Just asking because people might use the option to later realize that there are duplicated inserts due same-data not supporting the case?
No description provided.