Level / levelup

A wrapper for abstract-leveldown compliant stores, for Node.js and browsers.
MIT License
4.08k stars 266 forks source link

public levelup rc testing #472

Closed juliangruber closed 5 years ago

juliangruber commented 7 years ago

We should get as many people as possible to test levelup@2.0.0-rc1.

I'm thinking the following:

ralphtheninja commented 7 years ago

@juliangruber Maybe check projects on npm with rather frequent downloads that are based on levelup?

ralphtheninja commented 7 years ago

https://www.npmjs.com/browse/depended/levelup

Maybe pouchdb?

juliangruber commented 7 years ago

@juliangruber Maybe check projects on npm with rather frequent downloads that are based on levelup?

Great idea! Didn't check downloads, but those stood out to me:

juliangruber commented 7 years ago

But yeah let's rather write a script that checks download numbers. Or is there an interface for this already?

juliangruber commented 7 years ago

https://twitter.com/juliangruber/status/905149153810010112

vweevers commented 7 years ago

https://github.com/addaleax/npm-get-top-dependents

ralphtheninja commented 7 years ago

I think we should release an rc1 of level as well, before tweeting about it and check projects using it. In theory it's the easiest module to upgrade since it takes care of the details with encoding-down etc.

ralphtheninja commented 7 years ago

@juliangruber You could also take a look at some of the nice-registry modules, e.g. https://github.com/nice-registry/download-counts

ralphtheninja commented 7 years ago

@zeke How would you approach something like this? For example quering for modules that depend on level or levelup and maybe sort them based on download count?

zeke commented 7 years ago

How would you approach something like this?

git clone https://github.com/nice-registry/native-modules
cd native-modules
$EDITOR lib/check-package.js # customize for levelup
npm install
npm run collect

Then tweak render.js while the collector runs. Takes an hour or two usually.

At any time while the collector is running:

npm run render

sort them based on download count?

Yeah you could sort by download-count or dependent-count.

The stats in those packages are out of date, but they should provide a good approximation of popularity. @kikobeats actually inspired me this week to dust off download-counts and start working on a way to keep it automatically updated, so that should be done soon™...

ralphtheninja commented 7 years ago

https://github.com/Level/level/pull/52

ralphtheninja commented 7 years ago

@zeke So this would limit the query to native modules only right? If so, we prob want to check all modules (stand alone modules or applications).

The stats in those packages are out of date, but they should provide a good approximation of popularity.

Getting a rough estimate is perfectly fine!

doowb commented 7 years ago

I've been using this for a while to get the dependents of a module and the download counts:

https://github.com/doowb/dependents-cli

npm i -g dependents-cli
dependents levelup

This gist has the results from running that. It also shows the version of levelup being used in that dependent.

ralphtheninja commented 7 years ago

@doowb Cool! I just tried out dependents levelup --format json and dependents level --format json and it works really well.

levelup is used more than level by far

@juliangruber Going to take some work to change that :)

juliangruber commented 7 years ago

Nice, dependents-cli is exactly it!

I extended the list in the opening post, does that sound good to all?

ralphtheninja commented 7 years ago

Correction: level is used by more modules but dependents of levelup has more downloads.

Also, I think many of the levelup dependents are actually using leveldown, so they should switch from levelup to level.

ralphtheninja commented 7 years ago

Fwiw. I think apps are easier to test than libraries. @juliangruber Tried it with voltra yet?

zeke commented 7 years ago

cc @0x00A

ralphtheninja commented 7 years ago

v2.0.0-rc2 is out

juliangruber commented 7 years ago

Tweeted about new level: https://twitter.com/juliangruber/status/907246400194441217

MeirionHughes commented 7 years ago

I've added a repo to track compatible libs / adapters / plugins / etc as I didn't see a repo already doing this.

Is there a need to mark version compatibility (of third-party libs) with LevelUP major versions? i.e. whether third-party libs are compatible with LevelUP v2

juliangruber commented 7 years ago

I think modules should still be compatible with levelup@2 as long as they don't instantiate their own database instance (it gets passed in) and they don't omit any callbacks from async methods.

MeirionHughes commented 7 years ago

I will cc the guys that did the typings on definatly-typed for levelup when rc3 + typings is published and they can try it.

ralphtheninja commented 7 years ago

I will cc the guys that did the typings on definatly-typed for levelup when rc3 + typings is published and they can try it.

@MeirionHughes Just need to bump memdown dev-dep and we should be good to go?

MeirionHughes commented 7 years ago

need a patch on memdown first to have npm include the memdown.d.ts file on publish

ralphtheninja commented 7 years ago

Oh right. Doh, sorry.

ralphtheninja commented 7 years ago

I can fix.

MeirionHughes commented 7 years ago

@blittle @tarruda as you are listed on definatly-typed as editors for levelup@1 typings; I'd welcome a review of the new bundled types in levelup@2

juliangruber commented 7 years ago

I tested level@2.0.0-rc1 in voltra and nothing needed to be changed!

ralphtheninja commented 7 years ago

@juliangruber I so want to push the button for 2.0.0 and get it over with :wink:

juliangruber commented 7 years ago

I'm 100% for that!

MeirionHughes commented 7 years ago

@ralphtheninja possibly may need to change the typings for the batch stuff; especially with regard to level-sublevel (cc @dominictarr) because it has extra properties for the batch items. that said, sublevel looks like it consumes a levelup instance - so in theory it could use different types entirely.

tarruda commented 7 years ago

@blittle @tarruda as you are listed on definatly-typed as editors for levelup@1 typings; I'd welcome a review of the new bundled types in levelup@2

Why are T*Options passed as type parameters and not explicitly defined interfaces? Even if you'd like to allow users to create extensions, it still seems better to have explicit base option types that users can extend.

Also, why are TKey/TValue are type parameters to abstract leveldown even though leveldown only supports Buffer/string as keys and values? I suggest to have a look at leveldown DefinitelyTyped definitions for some ideas on how to be more strict: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/leveldown/index.d.ts .

An extra suggestion for levelup2 API change/addition, even though I think it would be too late at this point: Since we are changing to a promise-based API for get/put/del, why not also have a promise-based API for reading/processing bulk data? For example, here's a snippet of some interfaces I use in a project (omitted the get/put/del APIs):

export interface DB<TK, TV> {
    iterate(opts?: QueryOptions<TK>): Promise<Cursor<TK, TV>>;
}
export interface Cursor<TK, TV> {
    nextRecord(): Promise<[TK, TV]>;
}

This query API wraps over levelup streaming API and allows usage like this:

async function printData () {
  const cursor = await db.iterate()
  for (;;) {
    const [key, value] = await cursor.nextRecord()
    console.log(key, value)
  }
}

This is implemented by wrapping levelup streaming API into something that lazily fetch rows by -once-listening to "readable" and "end" events (to simplify cleanup), but it would be simpler to wrap directly over leveldown iterator API.

MeirionHughes commented 7 years ago

Why are T*Options passed as type parameters and not explicitly defined interfaces? Even if you'd like to allow users to create extensions, it still seems better to have explicit base option types that users can extend.

The main reason is because levelup is basically a proxy for whatever you pass to it and all options are passed the underlying db. So the typings for levelup methods need to come from the base db. Having the generics means typescript can infer those options automatically. For example, if you do:

let db = levelup(encode(leveldown('./db')));

then db is typed:

type DbType = levelup.LevelUp<
  any,
  any,
  leveldown.LevelDownOptions,
  leveldown.LevelDownPutOptions & encoding.CodecOptions,
  leveldown.LevelDownGetOptions & encoding.CodecOptions,
  leveldown.LevelDownDeleteOptions & encoding.CodecOptions,
  leveldown.LevelDownIteratorOptions<any, any> & encoding.CodecOptions,
  leveldown.LevelDownBatchOptions & encoding.CodecOptions>

Or if you use RocksDB instead, without using the encode adapter.

type DbType = levelup.LevelUp<
  any,
  any,
  rocksdb.RocksDbOptions,
  rocksdb.RocksDbPutOptions,
  rocksdb.RocksDbGetOptions,
  rocksdb.RocksDbDeleteOptions ,
  rocksdb.RocksDbIteratorOptions,
  rocksdb.RocksDbBatchOptions

I'm not a fan of all these generics myself either, but I couldn't see another way infer the base database without then.

Also, why are TKey/TValue are type parameters to abstract leveldown even though leveldown only supports Buffer/string as keys and values?

I guess potentially an argument could be made that any new abstract-leveldown could accept different types other than Buffer / string (i.e. typed arrays). Having the generics also comes back to getting levelup correctly typed via inference. Also, abstract-leveldown acts as middleware too; i.e. encoding-leveldown implements abstract-leveldown and so TKey and TValue can be anything there.

There are some short comings with all this though; as outlined here.

I suggest to have a look at leveldown DefinitelyTyped definitions for some ideas on how to be more strict: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/leveldown/index.d.ts .

with regard to leveldown itself, I could remove the ability to retype TKey and TValue and implement AbstractLevelDown<string | Buffer, string | Buffer, ... >, and also simplify the iterator typing too.

juliangruber commented 7 years ago

@ralphtheninja possibly may need to change the typings for the batch stuff; especially with regard to level-sublevel (cc @dominictarr) because it has extra properties for the batch items. that said, sublevel looks like it consumes a levelup instance - so in theory it could use different types entirely.

This is a huge warning sign to me, the typings shouldn't be a problem for a single module consuming levelup.

juliangruber commented 7 years ago

@ralphtheninja we should hold off the release.

Also I wouldn't be interested in having to do a new major release when typings change.

What about this instead: We publish a separate level*-typed and only publish officially once this is stable.

MeirionHughes commented 7 years ago

@juliangruber currently if you simply use level-sublevel without @types/level-sublevel it works perfectly fine. However, because @types/level-sublevel is currently typed with the levelup@1 types off dt , it will break for anyone using typescript if they switch up to levelup@2 + @types/level-sublevel.

image

What I can possibly do is add all these old levelup@1 types from dt in to levelup@2's namespace as aliases, and hopefully not break anything for ts users. I don't know how far to go down that route though; the typings for levelup@1 include things like encoding and options for things that I believe where all moved out to the underlying db - which only needs to implement AbstractLevelDown but can (in principle) take different options all-together.

MeirionHughes commented 7 years ago

okay; I've mitigated the typings with a non-breaking change so that it plays nice with the current level-sublevel typings - which need to updated anyway to add the promise stuff.

import 'levelup-async-iterator';
import * as levelup from 'levelup';
import * as leveldown from 'leveldown';
import * as encode from 'encoding-down';
import * as Sublevel from 'level-sublevel';

let up = levelup(encode(leveldown('./db')))
let db = Sublevel(up);
let sub = db.sublevel('stuff');

async function main() {
  //put a key into main!  
  await new Promise(r => db.put("john", "doe", r));
  //put a key into the sub-section!  
  await new Promise(r => sub.put("hello", "world", r));

  await new Promise(r =>
    db.batch([{ type: 'put', key: 'mike', value: 'powers' }], r));

  await new Promise(r =>
    sub.batch([{ type: 'put', key: 'jane', value: 'doe' }], r));

  let iter = up.iterator();

  for await (let [key, value] of iter) {
    console.log(key, value);
  }
}

main();
D:\Code\playground>ts-node --harmony_async_iteration main.ts
!!john doe
!!mike powers
!stuff!hello world
!stuff!jane doe
MeirionHughes commented 7 years ago

@ralphtheninja @juliangruber

What about this instead: We publish a separate level*-typed and only publish officially once this is stable.

I'm also leaning toward the idea of pulling out the typings into separate packages until they're stable and all third-party stuff has had a chance to upgrade. I really don't want to be the one that causes friction on such a large library base.

Its not a big deal for end-users, installing would just be:

yarn add levelup
yard add levelup-typings -D

or simply put them back into defiantly-typed for now.

when everything is stable you could potentially merge it back on the next major version.

that said; with compatible typings sublevel can also be fully typed and inferred from the base db chain...

image

juliangruber commented 7 years ago

Ok if you're not sure about this either, we should definitely go with a separate repo. If we merge this, we need to be very sure about it, as you said.

MeirionHughes commented 7 years ago

I think its a case of whether you're happy to wait a little longer before releasing v2, give people another week(s) to upgrade to the new way of doing things, and give feedback on v2 (and typing).

If you want to pull the trigger right now, then I'd say the types would need to be separated out.

That said, I'm in the process of getting a few minor prs ready that will resolve the remaining concerns I have with the Batch and Iterator Options. I've also added in some of the legacy types from version 1 types. So level-sublevel will still compile and run without any changes.

juliangruber commented 7 years ago

we can also just release v2.1 with the typings, they don't need to be added just for v2

ralphtheninja commented 7 years ago

we can also just release v2.1 with the typings, they don't need to be added just for v2

If we go this route, should we remove the current typings? If so, then we could just remove them for now and get 2.0.0 directly after that.

However, we should put some work into the README before the release imo.

juliangruber commented 7 years ago

I agree, let's remove the typings, just push what we have right now to a branch.

juliangruber commented 7 years ago

However, we should put some work into the README before the release imo.

Which readme's exaclty? There's two entry points (levelup and level), I feel already that is confusing. In the end, a docs web page would be super sweet.

MeirionHughes commented 7 years ago

I've branched master to 2.1 and I'll pr to reverse the typings from master

ralphtheninja commented 7 years ago

Which readme's exaclty? There's two entry points (levelup and level), I feel already that is confusing. In the end, a docs web page would be super sweet.

@juliangruber Imo both level and levelup readmes. But I like the idea of a docs web page. I think we should spend some time on leveldb.org page. Make it super clean.

ralphtheninja commented 7 years ago

I'm releasing 2.0.0 today if no one is against it!

ralphtheninja commented 7 years ago

@juliangruber Also see https://github.com/Level/level/issues/9

MeirionHughes commented 7 years ago

@ralphtheninja could you try a custom encoder with the latest + rc. I'm not sure where I'm going wrong here:

const leveldown = require('leveldown');
const encoding = require('encoding-down')
const levelup = require('levelup');

let db = levelup(
  encoding(leveldown('./db'), {
    keyEncoding: {
      encode: function (a) { return a.name },
      decode: function (b) { return { name: b } },
      buffer: false,
      type: "custom"
    }
  })
);

async function main() {
  await db.put({ name: "Hello" }, "World");
  await db.get({ name: "Hello" });
}

main().catch(console.log);

WriteError: key cannot be null or undefined at D:\up\node_modules\levelup\lib\levelup.js:183:23 at LevelDOWN.AbstractLevelDOWN.put (D:\up\node_modules\abstract-leveldown\abstract-leveldown.js:108:12) at DB._put (D:\up\node_modules\encoding-down\index.js:42:11) at DB.AbstractLevelDOWN.put (D:\up\node_modules\abstract-leveldown\abstract-leveldown.js:117:17) at D:\up\node_modules\deferred-leveldown\deferred-leveldown.js:23:27 at Array.forEach () at D:\up\node_modules\deferred-leveldown\deferred-leveldown.js:22:22 at D:\up\node_modules\abstract-leveldown\abstract-leveldown.js:42:7 at D:\up\node_modules\abstract-leveldown\abstract-leveldown.js:42:7

node v8.5.0