Fixes for AbstractLevelDown 2

[mcollina]

This partially solves the problems with AbstractLevelDown 2 (at least the initialization)

Unfortunately, the new feature "support for null values", needs to be implemented in a not-backward compatible way. Basically IDBWrapper returns null if a key is not found, so it is impossible to tell if the specified value is null or if there is no key.

I am not sure if exposing this difference in IDBWrapper is possible, and I had no time to look at this issue further

I recommend:

1. Release 2.2.1 with the old AbstractLevelDown
2. Fix the NULL value support and release it as level-js 3.

Also cc @JamesKyburz and @jprichardson @juliangruber

[juliangruber]

lgtm

i guess for the null value problem we could use some special string with escaping

[mcollina]

Yes. Or wrap everything in a simple object. Would you like to assemble
something?
Il giorno dom 5 lug 2015 alle 16:31 Julian Gruber [email protected]
ha scritto:

lgtm

i guess for the null value problem we could use some special string with
escaping

—
Reply to this email directly or view it on GitHub
#41 (comment).

[max-mapper]

ok 2.2.1 is out with "abstract-leveldown": "~0.12.0". once this is ready we can do a 3.0.0

[juliangruber]

i started fixing bugs, i'll post my progress later today

[mcollina]

I've opened an issue against IDBWrapper, maybe we should just add a method/argument there.

[jensarps]

@mcollina Going through old IDBWrapper issues, and found this: jensarps/IDBWrapper#63

Feel free to get back to me over there!

[timkuijsten]

To complement jensarps comment on Chrome, I've noticed Firefox 46.0a2 can return undefined as well, so not only null. See this patch timkuijsten@18965a8

[timkuijsten]

Based on PR #51 I've started testing with abstract-leveldown2 and found that most tests succeed. I've fixed all remaining failing tests. I cannot reproduce the problems with undefined versus null values, Firefox 46.0a2 does return undefined as well as Chrome, not null for values that do not exist.

Some abstract tests require to differentiate between undefined as a value and not exists. The IndexedDB API has a note explaining openCursor can be used for this instead of get. Therefore I'm using IDB.iterate with a keyRange in _get now.

The work is ready to be merged and lives in https://github.com/timkuijsten/level.js/tree/abstract-level2. I'm currently proceeding with #46 and I'm not sure if I should already open up a PR for bumping the deps to abstract-level2 or wait until #46 is finished?

[mcollina]

this would be a major anyway, if #46 is close by, let's just jump there.

Also, it would be really good to have zuul+saucelabs to run tests on all
browsers.
Il giorno sab 6 feb 2016 alle 17:30 Tim Kuijsten [email protected]
ha scritto:

Based on PR #51 #51 I've
started testing with abstract-leveldown2 and found that most tests succeed.
I've fixed all remaining failing tests. I cannot reproduce the problems
with undefined versus null values, Firefox 46.0a2 does return undefined as
well as Chrome, not null for values that do not exist.

Some abstract tests require
https://github.com/Level/abstract-leveldown/blob/master/abstract/put-get-del-test.js#L141
to differentiate between undefined as a value and not exists. The IndexedDB
API has a note explaining openCursor can be used for this instead of get.
Therefore I'm using IDB.iterate with a keyRange in _get now
timkuijsten@7ee8b6c
.

The work is ready to be merged and lives in
https://github.com/timkuijsten/level.js/tree/abstract-level2. I'm
currently proceeding with #46
#46 and I'm not sure if I
should already open up a PR for bumping the deps to abstract-level2 or wait
until #46 #46 is finished?

—
Reply to this email directly or view it on GitHub
#41 (comment).

[mvayngrib]

@timkuijsten i'm playing with your branch, and though the tests do pass, levelup's createReadStream never gets past the first value (and never emits 'end'). Could you take a look? I added the test to this fork https://github.com/mvayngrib/level.js/tree/al2:

  tape.only('read stream', function (t) {
    var level = levelup('read-stream-test', {db: leveljs})
    var batch = new Array(2).fill(null).map(function (whatever, i) {
      return {
        type: 'put',
        key: '' + i,
        value: '' + i
      }
    })

    level.batch(batch, function (err) {
      if (err) throw err

      const data = []
      level.createReadStream()
        .on('data', function (item) {
          data.push(item)
        })
        .on('error', t.error)
        .on('end', function () {
          t.same(data, batch.map(function (row) {
            return {
              key: row.key,
              value: row.value
            }
          }))

          t.end()
        })
    })
  })

[timkuijsten]

Hi @mvayngrib, I can reproduce that your test doesn't finish on the abstract-leveldown2 branch. However, it should be noted that most of my effort went into the idbunwrapper branch. I've tested your test on that branch and it does pass successfully.

I'm starting to use the idunwrapper code more and more, so far so good. Maybe you can try out that branch? https://github.com/timkuijsten/level.js/tree/idbunwrapper

[mvayngrib]

@timkuijsten actually i tried the idbunwrapper branch first :) The problem I had there was unsupported Buffer keys.

[mvayngrib]

@timkuijsten scratch that, i don't experience that issue (with Buffer keys) anymore, must have been a bug on my side. Will get back to you if something doesn't work for me

[timkuijsten]

@mvayngrib just pushed a big update into my idbunwrapper branch that could use some extra testing if you'd like.

[mvayngrib]

@timkuijsten great, i'll give it a try!

[ralphtheninja]

@vweevers Can we close this as is or do you want to supersede it by something first?

[vweevers]

Yeah, we'll get to this "automatically".