Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update dependencies #39

Merged
merged 10 commits into from
Jun 1, 2018
Merged

Update dependencies #39

merged 10 commits into from
Jun 1, 2018

Conversation

ralphtheninja
Copy link
Member

@ralphtheninja ralphtheninja commented May 31, 2018

Closes #9
Closes #24
Closes #22
Supersedes #7

Squash when merge.

@ralphtheninja
Copy link
Member Author

What's left now are some minor cleanup, docs, changelog, upgrade guide etc.

@ralphtheninja
Copy link
Member Author

And later on refactoring to get rid of down() function.

leveldown.js Outdated
@@ -83,7 +83,8 @@ SubDown.prototype._open = function (opts, cb) {
return done()
}

this.db.on('open', this.open.bind(this, opts, done))
this.db.once('open', this.open.bind(this, opts, done))
this.db.open()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the open, close, open, close test in abstract-leveldown (which @finnp commented out in previous PR)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should tweak it to do if (this.db.isClosed()) { this.db.open() }?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can simplify the whole thing and take advantage of the fact that levelup#open does process.nextTick(callback) if already open.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ralphtheninja would this work?

SubDown.prototype._open = function (opts, cb) {
  var self = this

  this.db.open(function (err) {
    if (err) return cb(err)

    var subdb = down(self.db, 'subleveldown')

    if (subdb && subdb.prefix) {
      self.prefix = subdb.prefix + self.prefix
      self.leveldown = down(subdb.db)
    } else {
      self.leveldown = down(self.db)
    }

    if (self._beforeOpen) self._beforeOpen(cb)
    else cb()
  })
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ralphtheninja ping in case you missed this because GH hides it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much cleaner! Let me try it out.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@vweevers
Copy link
Member

I'll review tomorrow.

test/index.js Outdated
@@ -65,25 +65,26 @@ test('SubDown constructor', function (t) {
test('SubDb main function', function (t) {
t.test('opts.open hook', function (t) {
t.plan(1)
subdb(levelup('loc', {db: memdown}), 'test', {
subdb(levelup(memdown('loc')), 'test', {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can remove the location argument (5x)

Copy link
Member

@vweevers vweevers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@vweevers
Copy link
Member

vweevers commented Jun 1, 2018

Could _serializeKey and _serializeValue be(come) a problem?

@ralphtheninja
Copy link
Member Author

@vweevers You mean the default in abstract-leveldown?

AbstractLevelDOWN.prototype._serializeKey = function (key) {              
  return Buffer.isBuffer(key) ? key : String(key)                         
}                                                                         
                                                                          
AbstractLevelDOWN.prototype._serializeValue = function (value) {          
  if (value == null) return ''                                            
  return Buffer.isBuffer(value) || process.browser ? value : String(value)
}                                                                         

@ralphtheninja
Copy link
Member Author

ralphtheninja commented Jun 1, 2018

_serializeKey looks ok for Buffer case but might be a problem with keys that aren't strings, OTOH concat doesn't care about prefix if it's not a string or a Buffer:

var concat = function (prefix, key, force) {                               
  if (typeof key === 'string' && (force || key.length)) return prefix + key
  if (Buffer.isBuffer(key) && (force || key.length)) {                     
    return Buffer.concat([Buffer.from(prefix), key])                       
  }                                                                        
  return key                                                               
}                                                                          

So for sub levels to work at all it seems to me that SubDb requires keys to be string or Buffer

@ralphtheninja
Copy link
Member Author

ralphtheninja commented Jun 1, 2018

@vweevers Also, remember that levelup + encoding-down is in front of all these ._put(), ._get() (etc) operations. So as long as the encoding from levelup -> subdown makes keys as strings or buffers, we should be good.

@ralphtheninja ralphtheninja merged commit c08f32c into master Jun 1, 2018
@ralphtheninja ralphtheninja deleted the update-dependencies branch June 1, 2018 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants