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 abstract-leveldown to the latest version 🚀 #32

Merged
merged 4 commits into from
Jan 28, 2018

Conversation

greenkeeper[bot]
Copy link
Contributor

@greenkeeper greenkeeper bot commented Jan 20, 2018

Closes #33

Version 4.0.0 of abstract-leveldown was just published.

Dependency abstract-leveldown
Current Version 3.0.0
Type dependency

The version 4.0.0 is not covered by your current version range.

If you don’t accept this pull request, your project will work just like it did before. However, you might be missing out on a bunch of new features, fixes and/or performance improvements from the dependency update.

It might be worth looking into these changes and trying to get this project onto the latest version of abstract-leveldown.

If you have a solid test suite and good coverage, a passing build is a strong indicator that you can take advantage of these changes directly by merging the proposed change into your project. If the build fails or you don’t have such unconditional trust in your tests, this branch is a great starting point for you to work on the update.


Commits

The new version differs by 50 commits.

  • 0d942fd 4.0.0
  • 18319ac add node 9 to travis
  • cdc560c prepare changelog for new release
  • 1656bcd add UPGRADING.md (#188)
  • 767fbdb remove .jshintrc
  • 0bd84a8 add @vweevers to contributors
  • 71bd174 Update changelog (#181)
  • bcdc953 Merge pull request #180 from ralphtheninja/master
  • 6f5a3d9 normalize readme
  • 48707e1 Merge pull request #178 from Level/remove-is-leveldown
  • 97938f1 :fire: remove is-leveldown
  • 54db15f Setup iterator options (#176)
  • 72fcae9 Merge pull request #175 from Level/test-common-defaults
  • 2ed07e5 :fire: remove testBuffer
  • 3f24eba make testCommon.js the default value for testCommon parameter

There are 50 commits in total.

See the full diff

FAQ and help

There is a collection of frequently asked questions. If those don’t help, you can always ask the humans behind Greenkeeper.


Your Greenkeeper bot 🌴

@greenkeeper greenkeeper bot added the dependencies Pull requests that update a dependency file label Jan 20, 2018
@ralphtheninja
Copy link
Member

ralphtheninja commented Jan 22, 2018

Added tweak should make #33 work

@vweevers I tried testing this locally on levelup but if I do npm link deferred-leveldown I get some error (it works if I point out this branch directly). Iirc you had a similar issue (I forget where it was) where you had problems with npm link. Does it ring a bell?


function DeferredLevelDOWN (db) {
AbstractLevelDOWN.call(this, '')
this._db = db
this._operations = []
this._iterators = []
closed(this)
Copy link
Member

Choose a reason for hiding this comment

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

Note that I moved this from the prototype to this since we need to know the properties of db to determine if approximateSize is present or not.

return this._db[m].apply(this._db, arguments)
}
})
if (self._db.approximateSize) {
Copy link
Member

Choose a reason for hiding this comment

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

This point of the code could be a seam for applying a manifest when opening

this._operations.push({ method: m, args: arguments })
}
})
obj._iterator = function (options) {
if (typeof self._db.approximateSize === 'function') {
Copy link
Member

Choose a reason for hiding this comment

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

This point of the code could be a seam for applying a manifest when closing

@ralphtheninja
Copy link
Member

If anyone feels like adding more tests to this branch. Please go ahead.

@vweevers
Copy link
Member

vweevers commented Jan 23, 2018

I tried testing this locally on levelup but if I do npm link deferred-leveldown I get some error (it works if I point out this branch directly). Iirc you had a similar issue (I forget where it was) where you had problems with npm link. Does it ring a bell?

@ralphtheninja I had two issues with npm link: typescript (fails to compile, Level/memdown#101) and browserify (throws error about Buffer, Level/abstract-leveldown#172).

it works if I point out this branch directly

Same for me.

@@ -41,29 +42,40 @@ DeferredLevelDOWN.prototype._close = function (callback) {
})
}

function open (obj) {
function open (self) {
Copy link
Member

Choose a reason for hiding this comment

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

+1 on renaming obj

}

function closed (obj) {
function closed (self, db) {
Copy link
Member

Choose a reason for hiding this comment

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

The db argument is unused?

Copy link
Member

Choose a reason for hiding this comment

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

Good catch. I originally added it but later changed to self._db inside the function.

@vweevers
Copy link
Member

If anyone feels like adding more tests to this branch. Please go ahead.

Let's add tests for stores with and without approximateSize.

@ralphtheninja
Copy link
Member

Let's add tests for stores with and without approximateSize.

Updated.

@ralphtheninja ralphtheninja added this to In progress in Level (old board) Jan 25, 2018
@ralphtheninja ralphtheninja moved this from In progress to Review in Level (old board) Jan 27, 2018
@ralphtheninja
Copy link
Member

major or minor?

@ralphtheninja ralphtheninja merged commit 346cf39 into master Jan 28, 2018
Level (old board) automation moved this from Review to Done Jan 28, 2018
@greenkeeper greenkeeper bot deleted the greenkeeper/abstract-leveldown-4.0.0 branch January 28, 2018 18:08
@ralphtheninja
Copy link
Member

@vweevers I'm thinking we should release new majors for everything that's inheriting from abstract-leveldown@4, i.e. deferred-leveldown, leveldown and encoding-down. Even though deferred-leveldown and encoding-down are special "stores".

@vweevers
Copy link
Member

@ralphtheninja that would be the safer route, yes. Otherwise we have to carefully look at each one to see if the changes in abstract-leveldown affect it in a breaking way.

@ralphtheninja
Copy link
Member

ralphtheninja commented Feb 2, 2018

I think we'll be fine to release deferred-leveldown without bumping major since it's the actual store passed to it in the constructor that really matters. So I'm thinking a patch will be fine.

That said, thinking is not good enough, so it would be nice to get some more information on how this works. I'll start with some npm link galore locally. Would it be enough if levelup-tests worked fine with a linked deferred-leveldown?

@ralphtheninja
Copy link
Member

Ok, I've tested level and levelup by using npm link and things work fine. I'd like to start releasing deferred-leveldown since levelup depends on it (and then level-packager and later level).

I think I want to release new major versions of deferred-leveldown and encoding-down after all, since they technically are implementations that inherits breaking changes from abstract-leveldown, even though they aren't stores themselves.

@vweevers @juliangruber WDYT?

@vweevers
Copy link
Member

vweevers commented Feb 8, 2018

since they technically are implementations that inherits breaking changes

Agree. Standing on principle removes the need to debate cases like these.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Remove approximateSize test
2 participants