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

Remove leveldown dependency #49

Closed
heri16 opened this issue Jun 3, 2018 · 19 comments

Comments

Projects
None yet
3 participants
@heri16
Copy link
Contributor

commented Jun 3, 2018

Would like to use nanosql with RocksDB, which is a successor of Leveldown. Wonder whether we could remove the leveldown dependency to further reduce package size.

@ClickSimply

This comment has been minimized.

Copy link
Owner

commented Jun 4, 2018

LevelDown doesn't affect the size of webpack/browser bundles and the like, node/webpack bundle a different index file for the server and browser, the browser version doesn't include the LevelDown dependencies.

As for removing it from the package.json as a dependency to reduce the download size for installs, I think the downsides to doing that far outweigh any positives. You wouldn't be able to just "plug and play" nanoSQL with NodeJS and get a persistent database anymore. This would also break anyone installing the update from a fresh install since LevelDown would no longer be downloaded like it was before (which is now expected behavior).

All that being said I just pushed 1.6.4, you can use RocksDB or any other LevelDown compatible adapter with the below method:

import { _LevelStore } from "nano-sql/lib/database/adapter-levelDB";
import * as LevelDown from "leveldown";

nSQL()
... data models and such
.config({
    mode: new _LevelStore("", 0, 0, LevelDown())
})
.connect()..

Let me know if you run into any further issues!

@ClickSimply

This comment has been minimized.

Copy link
Owner

commented Jun 6, 2018

Since there's been no response to this issue for a few days I'm going to close it. Feel free to open it back up if you have further questions.

@ClickSimply ClickSimply closed this Jun 6, 2018

@heri16

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2018

npm rebuild on my project takes a long time due to leveldown, even though I don't intend to use leveldown.

Could we at least move it into peer Dependencies which seem to be an idiomatic way in npm@6 land (https://docs.npmjs.com/files/package.json#peerdependencies).

  • People who would like to use the defaults will automatically see a warning by npm to install leveldown.
  • People who would not like to use the default (e.g. use Google Datastore) will just create a stub package on github that is named leveldown but contains nothing. if leveldown is also added into optionalDependencies, this step might not even be required.

We could also add npm install express nano-sql leveldown to the example documentation.

@heri16

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2018

I don't think OP have been given the rights to reopen issues.

@ClickSimply ClickSimply reopened this Jun 11, 2018

@heri16

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2018

@ClickSimply the suggested code did not work for me.

Sample code:

const { nSQL } = require('nano-sql')
const { _LevelStore } = require('nano-sql/lib/database/adapter-levelDB')
const rocksdb = require('rocksdb')
const leveldown = require('leveldown')

const fs = require('fs')

if (!fs.existsSync('/tmp/db_eqh')) {
  fs.mkdirSync('/tmp/db_eqh')
}

// Declare Table
nSQL('posts').model([
  { key: 'id', type: 'int', props: ['pk', 'ai'] },
  { key: 'title', type: 'string' },
  { key: 'content', type: 'string' },
  { key: 'date', type: 'int' },
]).config({
  id: 'eqh', // namespace of the underlying table
  mode: new _LevelStore('/tmp', 32, 32, rocksdb('/tmp/db_eqh/posts')), // AWS Lambda only has one writable directory
  history: 'row', // store each row's changes as a revision history
  // mode: 'PERM', // store changes permenantly
  // cache: false, // dont use javascript object cache
})

Error:

OpenError: IO error: lock /tmp/db_eqh/posts/LOCK: No locks available
OpenError: IO error: lock /tmp/db_eqh/posts/LOCK: already held by process
@ClickSimply

This comment has been minimized.

Copy link
Owner

commented Jun 12, 2018

Hello Heri,
I updated the api again for custom leveldown adapters, the example below should work with 1.6.7

const { nSQL } = require("nano-sql");
const { _LevelStore } = require("nano-sql/lib/database/adapter-levelDB");
const rocksdb = require("rocksdb");
const fs = require("fs");
const path = require("path");

// Declare Table
nSQL('posts').model([
    { key: 'id', type: 'int', props: ['pk', 'ai'] },
    { key: 'title', type: 'string' },
    { key: 'content', type: 'string' },
    { key: 'date', type: 'int' },
]).config({
    id: 'eqh', // namespace of the underlying table
    mode: new _LevelStore((databaseID, tableName) => {
        const basePath = path.join(__dirname, "db_" + databaseID);
        if (!fs.existsSync(basePath)) {
            fs.mkdirSync(basePath);
        }
        return {
            lvld: rocksdb(path.join(basePath, tableName)),
            args: {} // arguments object to pass into LevelUp function as second argument.
        };
    }),
    history: 'row', // store each row's changes as a revision history
    // mode: 'PERM', // store changes permenantly
    // cache: false, // dont use javascript object cache
}).connect().then(() => {
    return nSQL().query("upsert", {title: "hello"}).exec();
}).then(() => {
    return nSQL().query("select").exec();
}).then((rows) => {
    console.log(rows);
})

I totally get not wanting to deal with leveldown in the build process if you're not using it, the alternatives provided would inconvenience current users of the library in ways I'd like to avoid. For example, peerDependencies don't get installed by default as you suggested meaning someone would have to run npm i levelup before using the LevelDB features. If we're going to do that, might as well split it out into it's own adapter library so the behavior is more verbose and consistent with the rest of the library.

Still, I'd like to keep around mode: "PERM" working in NodeJS without any other installs needed so dropping LevelDB from the package we'd need to add an alternative of some kind back in.

How often do you call npm rebuild? My understanding is this process is needed infrequently. It might be more reasonable to put together a small script that prunes the leveldown dependency from nanoSQL in your node_modules folder after install, something like this:

const fs = require("fs");
const path = require("path");

const packageJSON = path.join(__dirname, "node_modules", "nano-sql", "package.json");
fs.readFile(packageJSON, (err, file) => {
    if (err) throw err;
    const package = JSON.parse(file.toString());
    delete package.dependencies.leveldown;
    fs.writeFile(packageJSON, JSON.stringify(package, null, 4), (err) => {
        if (err) throw err;
    })
});

Still, I wouldn't mind leaving this issue open for a while and seeing if other folks have feedback. If you're experience is a common one, that is people would prefer to do the extra step to get LevelDB rather than have it bundled I'm certainly open to it.

@heri16

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2018

The prune script sounds like a great idea.

Another nice way is to move leveldown into optionalDependencies and let users do npm install —no-optional .
See: https://docs.npmjs.com/cli/install

@ClickSimply

This comment has been minimized.

Copy link
Owner

commented Jun 14, 2018

Interesting, I didn't know you could use optional dependencies like that.

I think that makes everyone happy then, 1.6.8 was just pushed to NPM and includes all the NodeJS dependencies moved to the optionalDependencies property of package.json. 👍

@heri16

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2018

Hi @ClickSimply, I have a feeling that by including levelup into the optionalDependencies, you would also need to add it into peerDependencies too. If not how would nodejs resolve the path to levelup and int64-buffer when npm install --no-optional is used?

peerDependencies: {
  "levelup": "^2.0.2",
  "int64-buffer": "^0.1.10"
}
├─┬ nano-sql@1.6.8
│ ├── fuzzysearch@1.0.3
│ ├── UNMET OPTIONAL DEPENDENCY int64-buffer@0.1.10
│ ├─┬ UNMET OPTIONAL DEPENDENCY leveldown@4.0.1
│ │ ├── abstract-leveldown@5.0.0 deduped
│ │ ├── bindings@1.3.0 deduped
│ │ ├── fast-future@1.0.2 deduped
│ │ ├── nan@2.10.0 deduped
│ │ └── prebuild-install@4.0.0 deduped
│ ├─┬ UNMET OPTIONAL DEPENDENCY levelup@2.0.2
│ │ ├─┬ UNMET OPTIONAL DEPENDENCY deferred-leveldown@3.0.0
│ │ │ └─┬ UNMET OPTIONAL DEPENDENCY abstract-leveldown@4.0.3
│ │ │   └── xtend@4.0.1 deduped
│ │ ├─┬ UNMET OPTIONAL DEPENDENCY level-errors@1.1.2
│ │ │ └─┬ UNMET OPTIONAL DEPENDENCY errno@0.1.7
│ │ │   └── UNMET OPTIONAL DEPENDENCY prr@1.0.1
│ │ ├─┬ UNMET OPTIONAL DEPENDENCY level-iterator-stream@2.0.1
│ │ │ ├── inherits@2.0.3 deduped
│ │ │ ├── readable-stream@2.3.6 deduped
│ │ │ └── xtend@4.0.1 deduped
│ │ └── xtend@4.0.1 deduped
│ ├── levenshtein-edit-distance@2.0.2
│ ├── lie-ts@4.0.0
│ ├── metaphone@1.0.3
│ ├── prefix-trie-ts@0.0.4
│ ├─┬ queue@4.4.2
│ │ └── inherits@2.0.3
│ ├── really-small-events@1.1.0
│ └── stemmer@1.0.2

Let me just check on this and get back to you.

@heri16

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2018

Alright, i was wrong on this, nodejs module resolution seems alright.

No LevelStore

npm install --no-optional nano-sql

RocksDB

npm install int64-buffer levelup@2
npm install --no-optional nano-sql
npm install rocksdb
@heri16

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2018

When leveldown not installed, crashed due to

global._leveldown = require("leveldown");

try {
  global._levelup = require("levelup");
} catch (err) {}
try {
  global._leveldown = require("leveldown");
} catch (err) {}
try {
  global._Int64BE = require("int64-buffer").Uint64BE;
} catch (err) {}

Would you like me to send a PR instead?

@ClickSimply

This comment has been minimized.

Copy link
Owner

commented Jun 14, 2018

Ah forgot about that. Sure, send it through and we'll hopefully get to close this issue for good soon! 😆

@heri16

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2018

Do we need a documentation update on the README?

const fs = require('fs')
const path = require('path')

const { nSQL } = require('nano-sql')
const { _LevelStore: LevelStore } = require('nano-sql/lib/database/adapter-levelDB')
const rocksdb = require('rocksdb')

nSQL().config({
  id: 'eqh', // namespace of the underlying table
  history: 'row', // store each row's changes as a revision history
  // dbPath: '/tmp', // Not required
  mode: new LevelStore((databaseID, tableName) => {
    const basePath = path.join('/tmp', `db_${databaseID}`)
    if (!fs.existsSync(basePath)) {
      fs.mkdirSync(basePath)
    }
    return {
      lvld: rocksdb(path.join(basePath, tableName)),
      // arguments to pass into LevelUp function as second argument.
      args: {
        cacheSize: 32 * 1024 * 1024,
        writeBufferSize: 32 * 1024 * 1024,
      },
    }
  })
})
@heri16

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2018

@ClickSimply Would you consider using level-packager instead to avoid the excessive custom configuration?

Exports a single function which takes a single argument, an abstract-leveldown compatible storage back-end for levelup. The function returns a constructor function that will bundle levelup with the given abstract-leveldown replacement.

Example code here: https://github.com/Level/level-rocksdb

const mode = require('rocksdb')
const levelFunc = require('level-packager')(mode)
@ClickSimply

This comment has been minimized.

Copy link
Owner

commented Jun 14, 2018

1.6.9 is live on NPM with the PR. 👍

@ClickSimply

This comment has been minimized.

Copy link
Owner

commented Jun 17, 2018

Closing this issue again, let me know if we still need to adjust things.

@bkniffler

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

Heya, I know Rocksdb is great performance wise, but I think having a build-in adapter that has native code shouldn't be part of the core. I, for example, have been bitten by

dyld: lazy symbol binding failed: Symbol not found: __ZN2v816FunctionTemplate3NewEPNS_7IsolateEPFvRKNS_20FunctionCallbackInfoINS_5ValueEEEENS_5LocalIS4_EENSA_INS_9SignatureEEEiNS_19ConstructorBehaviorE
  Referenced from: /Users/bkniffler/Git/diego/node_modules/rocksdb/build/Release/leveldown.node
  Expected in: flat namespace

dyld: Symbol not found: __ZN2v816FunctionTemplate3NewEPNS_7IsolateEPFvRKNS_20FunctionCallbackInfoINS_5ValueEEEENS_5LocalIS4_EENSA_INS_9SignatureEEEiNS_19ConstructorBehaviorE
  Referenced from: /Users/bkniffler/Git/diego/node_modules/rocksdb/build/Release/leveldown.node
  Expected in: flat namespace

And when switching to SQLite, my electronjs will always still build the Rocksdb native dependency.

So wouldn't it maybe make sense to have only memory as a default adapter?

@bkniffler

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

Oh, I could only bypass the error by referencing '@nano-sql/core/lib/index'

import { SQLite } from '@nano-sql/adapter-sqlite';
import { nSQL } from '@nano-sql/core/lib/index';
@ClickSimply

This comment has been minimized.

Copy link
Owner

commented Jan 30, 2019

The solution mentioned in this thread should also work for v2:

npm install --no-optional @nano-sql/core

All of the NodeJS specific dependencies are set as optional. Also pulling the index file used for browser builds as you suggested is another good solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.