Closed ralphtheninja closed 6 years ago
Rebased onto latest master. Needs docs too I guess.
Can you add a note to UPGRADING.md? We'll need to add type checks to leveldown
etc
Browser tests never started, restarting build.
Entertaining two thoughts, because I think we can afford to take bigger steps.
Thought 1. Make location
optional by checking arguments.length
. This gives implementations the choice of supporting location
without them having to do extra type checks. In addition, make the .location
property optional too?
function AbstractLevelDOWN (location) {
if (arguments.length > 0) {
if (typeof location !== 'string') {
throw new Error('constructor requires a location string argument')
}
this.location = location
}
this.status = 'new'
}
function MemDOWN () {
AbstractLevelDOWN.call(this)
}
function LevelDOWN (location) {
AbstractLevelDOWN.call(this, location)
}
function SQLDown (connection) {
this._connection = connection
AbstractLevelDOWN.call(this)
}
Thought 2. Remove location altogether, it's not that much code to move to leveldown
:
function AbstractLevelDOWN () {
this.status = 'new'
}
function MemDOWN () {
AbstractLevelDOWN.call(this)
}
function LevelDOWN (location) {
if (typeof location !== 'string') {
throw new Error('constructor requires a location string argument')
}
this._location = location
AbstractLevelDOWN.call(this)
}
function SQLDown (connection) {
this._connection = connection
AbstractLevelDOWN.call(this)
}
An additional benefit of either of these approaches is that we can check location !== ''
. Are empty locations handled in the c++?
Entertaining two thoughts, because I think we can afford to take bigger steps.
Good call!
I like both alternatives, but I think I prefer the second. Since some implementation aren't using location
I don't see why it should be stored in a base class at all. Also makes AbstractLevelDOWN
a lot cleaner.
Additionally, I think we might need to change how the abstract tests are invoked as well. If location
is removed from abstract-leveldown
and if implementations aren't necessarily using a location, then maybe we shouldn't do e.g.
var db = leveldown(testCommon.location())
but rather
var db = factory()
So implementations pass in factory
instead and can handle themselves how the instance should be created, with a location or not.
Aaah we're of course already using a factory, so we shouldn't pass testCommon.location()
to it.
A factory
can also support options (#227). Two birds one stone :)
Aaah we're of course already using a factory
So none of the current tests assume that the function has a prototype?
So none of the current tests assume that the function has a prototype?
I haven't checked all. With that you mean that we don't have to call new
?
With that you mean that we don't have to call new?
And that the tests don't do things like instanceof fn
@vweevers If you're cool with it, I'd like to close this PR and start over with removing location
.
Ps. It seems to me that test/leveldown-test.js
is getting more and more redundant since it will be only one test left which doesn't really test anything
Closes https://github.com/Level/abstract-leveldown/issues/63