Skip to content
This repository has been archived by the owner on Mar 23, 2023. It is now read-only.

fix: upgrade level libs to resolve node 10 failure #7

Merged
merged 6 commits into from
May 29, 2018
Merged

Conversation

jacobheun
Copy link
Contributor

@jacobheun jacobheun commented May 9, 2018

This upgrades the versions of leveldown, levelup and level-js to resolve a node 10.x build issue in ipfs-repo.

This also moves the responsibility of shiming leveldown with level-js for the browser. The version of level-js has been changed to match the current version in js-ipfs-repo.

I've run this against the ipfs-repo test suite, but still would like to run it against js-ipfs.

connects to ipfs/js-ipfs#1347
connects to ipfs/js-ipfs-repo#167
resolves #6

License: MIT
Signed-off-by: Jacob Heun <jacobheun@gmail.com>
@achingbrain
Copy link
Member

It would be great if this could get merged as I'm seeing Level/leveldown#194 in js-ipfs which seems to have been resolved but the fix was released in a later version.

Unverified

This user has not yet uploaded their public signing key.
License: MIT
Signed-off-by: Jacob Heun <jacobheun@gmail.com>
daviddias
daviddias previously approved these changes May 15, 2018
Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

tiny issue otherwise LGTM

src/index.js Outdated
}))
// Default to leveldown db
const database = opts && opts.db ? opts.db : require('leveldown')
delete opts.db
Copy link
Member

Choose a reason for hiding this comment

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

needs an existence check like the line above otherwise ReferenceError: opts is not defined for null/undefined

Copy link
Member

Choose a reason for hiding this comment

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

...and a test? ;)

Verified

This commit was signed with the committer’s verified signature.
BethGriggs Bethany Griggs
License: MIT
Signed-off-by: Jacob Heun <jacobheun@gmail.com>

Verified

This commit was signed with the committer’s verified signature.
BethGriggs Bethany Griggs
License: MIT
Signed-off-by: Jacob Heun <jacobheun@gmail.com>
@jacobheun
Copy link
Contributor Author

@alanshaw yes, thanks for pointing that out. I've added tests for default and override configuration. I've also added levedown to the interface test suites so we can ensure those pluggable databases are working properly with dependency upgrades.

@jacobheun
Copy link
Contributor Author

@alanshaw this should be good, can you take a look and approve if all looks good? This is being used in the js-ipfs node 10 fix which I have jenkins builds passing for https://github.com/ipfs/js-ipfs/pull/1358/files.

dignifiedquire
dignifiedquire previously approved these changes May 24, 2018
Copy link
Member

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

this looks good to me, but please add a note about who is responsible for shimming to the readme

alanshaw
alanshaw previously approved these changes May 24, 2018

Verified

This commit was signed with the committer’s verified signature.
BethGriggs Bethany Griggs
License: MIT
Signed-off-by: Jacob Heun <jacobheun@gmail.com>
@jacobheun jacobheun dismissed stale reviews from alanshaw and dignifiedquire via 6e7ebe0 May 24, 2018 16:01
@jacobheun
Copy link
Contributor Author

@dignifiedquire thanks for pointing that out, I updated the readme to reference the shimming expectations. This should be good to merge now, once that's verified and it's re-approved.

src/index.js Outdated
database(path),
Object.assign({}, opts, {
compression: false, // same default as go
valueEncoding: 'binary'

Choose a reason for hiding this comment

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

FYI levelup v2 no longer includes encodings, so this valueEncoding option will have no effect.

Instead, wrap database(path) with encoding-down. See UPGRADING.md for more.

License: MIT
Signed-off-by: Jacob Heun <jacobheun@gmail.com>
@jacobheun
Copy link
Contributor Author

Thanks for pointing that out @vweevers, I've updated the code to reflect the levelup upgrading guide and tested this against ipfs-repo. Pending reviews, this should good to go.

@daviddias daviddias merged commit a5d7378 into master May 29, 2018
@ghost ghost removed the status/in-progress In progress label May 29, 2018
@daviddias daviddias deleted the fix/node10 branch May 29, 2018 14:47
@jacobheun jacobheun mentioned this pull request May 29, 2018
10 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade leveldown?
6 participants