search

Level / abstract-leveldown

An abstract prototype matching the leveldown API.
MIT License
146 stars 53 forks source link

AbstractChainedBatch: pass options to _write #200

Closed vweevers closed 6 years ago

vweevers commented 6 years ago

Discovered in https://github.com/Level/abstract-leveldown/issues/193#issuecomment-359369932. If _write is defined, it does not receive the options, unlike _batch:

https://github.com/Level/abstract-leveldown/blob/0d942fd45df5c9915269ded6f0f76fdfade11575/abstract-chained-batch.js#L79-L83

Fixing this would be a breaking change. WDYT @ralphtheninja @juliangruber?

juliangruber commented 6 years ago

Would it be a breaking change, or rather a long outstanding bug fix? 🤔

vweevers commented 6 years ago

It's a breaking change per semver because implementations would have to change their function signature to _write(options, callback).

ralphtheninja commented 6 years ago

Hmm I think I've seen an issue on this before, but can't remember where. leveldown would not be affected though:

https://github.com/Level/leveldown/blob/c7e2b762de1b99e97d8a86b8d6328da33cec3dc9/chained-batch.js#L26-L28

I guess the c++ code must check for this.

vweevers commented 6 years ago

@ralphtheninja hmm wait, if leveldown's signature is already _write(options, callback).. why does that work?

vweevers commented 6 years ago

Ah, the C++ takes the first argument as a callback. So it would break leveldown.