Skip to content
This repository has been archived by the owner on Dec 1, 2024. It is now read-only.

feat: restore typings + testing #499

Closed
wants to merge 1 commit into from

Conversation

MeirionHughes
Copy link
Member

  • to start discussion and review of adding typings back into master
  • keen eye will notice imports for tests are using .default for some level stuff. This is because the typescript compiler will complain because those repositories now have es2015 default exports.

package.json Outdated
},
"scripts": {
"test": "standard && node test | faucet"
"test": "standard && ts-node --no-cache test | faucet"
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 it should be in addition to node tests. Tests passing in TS is not a guarantee for tests passing in node.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I can switch back to having both.

test/common.js Outdated

assert(levelup.errors === errors)
assert(leveluperrors === errors)
Copy link
Member

Choose a reason for hiding this comment

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

Is this a requirement for TS? I like the previous better

Copy link
Member Author

@MeirionHughes MeirionHughes Oct 4, 2017

Choose a reason for hiding this comment

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

unfortunately. without moving the test to ts (so you can trick the compiler) I'm not sure how to go around it. typescript treats it as an es2015 module.

The alternative would be to have specific typescript-based tests for common things - that way the typescript stuff would not interfere at all with the js tests. I don't mind doing that with something like mocha.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll have another stab; might be a way to add the errors onto the default too.

tsconfig.json Outdated
"compilerOptions": {
"target": "es2015",
"moduleResolution": "node",
"noEmitOnError": true,
Copy link
Member

Choose a reason for hiding this comment

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

Didn't notice this before. What does noEmitOnError mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

Generally a fail-safe to ensure it doesn't just warn then run, but fails completely. Its probably not needed as it defiantly was failing the test before without it.

Copy link
Member

Choose a reason for hiding this comment

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

OK so it's just a confusing name :)

package.json Outdated
@@ -33,35 +33,39 @@
"storage",
"json"
],
"main": "lib/levelup.js",
"main": "lib/levelup.js",
"typings": "lib/levelup.js",
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be lib/levelup.d.ts?

Copy link
Contributor

@huan huan left a comment

Choose a reason for hiding this comment

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

I'd like to suggest us using var levelup = require('levelup') instead of var levelup = require('levelup').default because they are all the same and the second one looks ugly. :-]

@@ -3,7 +3,7 @@
* MIT License <https://github.com/level/levelup/blob/master/LICENSE.md>
*/

var levelup = require('../lib/levelup.js')
var levelup = require('../lib/levelup.js').default
Copy link
Contributor

@huan huan Oct 4, 2017

Choose a reason for hiding this comment

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

I believe this is not necessary because we had exported both LevelUP and default.

See:

module.exports = LevelUP.default = LevelUP

That will make sure the code will pass the following assertion:

const levelup1 = require('../lib/levelup.js')
const levelup2 = require('../lib/levelup.js').default

assert(levelup1 === levelup2)

Copy link
Member Author

@MeirionHughes MeirionHughes Oct 4, 2017

Choose a reason for hiding this comment

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

Its down to typescript - you cannot have both export default and export = in the typings. So the typings have gone with the default export exclusively for typescript users.

It means when you run the normal js through the typescript compiler it will complain if you don't use the default

Copy link
Contributor

@huan huan Oct 4, 2017

Choose a reason for hiding this comment

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

Ok, understand.

Then how about we do a module.exports = LevelUP.default = LevelUP.LevelUP = LevelUP ?

Then we will be able to do a var { LevelUP } = require('../lib/levelup.js'), which would be better than require('...').default?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of require('...').default - but because it's only in the tests, I prefer it over more hacks in lib/levelup.js.

Copy link
Member Author

@MeirionHughes MeirionHughes Oct 4, 2017

Choose a reason for hiding this comment

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

yeah I don't think we need more hacks to the export; Its just for the test here because we're pushing the (mostly) unadulterated js through the compiler. normal javascript users:

const levelup = require('levelup')

normal ts users:

import levelup from 'levelup'

you can always do const {default:levelup} = require('levelup'); but I suspect we might run into issues if we're still testing on an older node.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with default too because it's just unit test script.

Copy link
Contributor

@huan huan left a comment

Choose a reason for hiding this comment

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

Looking forward to the new NPM version with typings!

README.md Outdated
@@ -24,6 +24,7 @@ LevelUP
* <a href="#events">Events</a>
* <a href="#extending">Extending LevelUP</a>
* <a href="#multiproc">Multi-process access</a>
* <a href="#typings">Typescript </a>
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: stylize like the official "TypeScript"? Same below

README.md Outdated
Typescript
----------

LevelUP comes with typescript definitions that can automatically infer options based on a typed `abstract-leveldown` implementation.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the simpler form: "infer options from a typed"

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@MeirionHughes
Copy link
Member Author

@zixia any chance you could pull/fork from me and test this locally?

if you do

yarn
yarn link

in the the local levelup folder it will be linkable in your own project. There you would use yarn link levelup. It would be super useful to make sure it all types correctly, for someone else.

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.

Maybe squash this

@MeirionHughes
Copy link
Member Author

@vweevers yeah, I'll squash and rebase after #498

@vweevers
Copy link
Member

vweevers commented Oct 5, 2017

Does the wiki page need updating?

@huan
Copy link
Contributor

huan commented Oct 6, 2017

No problem, will do it later today.

@vweevers
Copy link
Member

vweevers commented Oct 6, 2017

No need, looks like @MeirionHughes already did

@huan
Copy link
Contributor

huan commented Oct 6, 2017

@MeirionHughes I use your repository to install levelup, and everything looks good.

It passed my tslint, rollup(by npm run dist) and passed all my unit tests as well.

My commit to replace levelup to your git repo: huan/flash-store@c9e1253

P.S. I think we should consider to create a new repository for TypeScript typing testing, because all the level-* modules sometimes might run into typing mismatch problem. It should be tested automatically.

See the details:
┌ zixia@zixia-desktop:~/git/flash-store [18:33:10] tty:[0] jobs:[0]
└ (MeirionHughes) $ grep MeirionHughes package.json 
    "levelup": "MeirionHughes/levelup",
┌ zixia@zixia-desktop:~/git/flash-store [18:33:13] tty:[0] jobs:[0]
└ (MeirionHughes) $ npm run lint

> flash-store@0.0.20 lint /home/zixia/git/flash-store
> tslint --project tsconfig.json && npm run clean && tsc --noEmit


> flash-store@0.0.20 clean /home/zixia/git/flash-store
> shx rm -fr dist/* bundles/*

┌ zixia@zixia-desktop:~/git/flash-store [18:33:18] tty:[0] jobs:[0]
└ (MeirionHughes) $ npm run dist

> flash-store@0.0.20 dist /home/zixia/git/flash-store
> npm run clean && npm run build && npm run rollup && npm run dist:es6to5


> flash-store@0.0.20 clean /home/zixia/git/flash-store
> shx rm -fr dist/* bundles/*


> flash-store@0.0.20 build /home/zixia/git/flash-store
> tsc --module es6


> flash-store@0.0.20 rollup /home/zixia/git/flash-store
> rollup -c

(!) Some options have been renamed
https://gist.github.com/Rich-Harris/d472c50732dab03efeb37472b08a3f32
options.entry is now options.input
options.moduleName is now options.name
options.sourceMap is now options.sourcemap
options.dest is now options.output.file
options.format is now options.output.format

dist/flash-store.js → bundles/flash-store.es6.umd.js...
(!) Mixing named and default exports
Consumers of your bundle will have to use bundle['default'] to access the default export, which may not be what you want. Use `exports: 'named'` to disable this warning
(!) Missing shims for Node.js built-ins
Creating a browser bundle that depends on 'path'. You might need to include https://www.npmjs.com/package/rollup-plugin-node-builtins
(!) `this` has been rewritten to `undefined`
https://github.com/rollup/rollup/wiki/Troubleshooting#this-is-undefined
dist/flash-store.js
1: var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) {
                    ^
2:     return new (P || (P = Promise))(function (resolve, reject) {
3:         function fulfilled(value) { try { step(generator.next(value)); } catch (e) { reject(e); } }
...and 9 other occurrences
(!) Unresolved dependencies
https://github.com/rollup/rollup/wiki/Troubleshooting#treating-module-as-external-dependency
path (imported by dist/flash-store.js)
app-root-path (imported by dist/flash-store.js)
brolog (imported by dist/flash-store.js)
rimraf (imported by dist/flash-store.js)
encoding-down (imported by dist/flash-store.js)
leveldown (imported by dist/flash-store.js)
levelup (imported by dist/flash-store.js)
(!) Missing global variable names
Use options.globals to specify browser global variable names corresponding to external modules
path (guessing 'path')
app-root-path (guessing 'appRootPath')
brolog (guessing 'brolog')
rimraf (guessing 'rimrafProxy')
encoding-down (guessing 'encodingProxy')
leveldown (guessing 'leveldownProxy')
levelup (guessing 'levelupProxy')
created bundles/flash-store.es6.umd.js in 45ms

> flash-store@0.0.20 dist:es6to5 /home/zixia/git/flash-store
> tsc --out ./bundles/flash-store.umd.js --target es5 --allowJs bundles/flash-store.es6.umd.js --lib es6,dom

┌ zixia@zixia-desktop:~/git/flash-store [18:33:23] tty:[0] jobs:[0]
└ (MeirionHughes) $ npm t

> flash-store@0.0.20 test /home/zixia/git/flash-store
> npm run lint && npm run test:unit


> flash-store@0.0.20 lint /home/zixia/git/flash-store
> tslint --project tsconfig.json && npm run clean && tsc --noEmit


> flash-store@0.0.20 clean /home/zixia/git/flash-store
> shx rm -fr dist/* bundles/*


> flash-store@0.0.20 test:unit /home/zixia/git/flash-store
> blue-tape -r ts-node/register -r source-map-support/register "src/**/*.spec.ts" "tests/**/*.spec.ts"

TAP version 13
# constructor()
ok 1 should not throw exception with a non existing workDir
# Store as iterator
# async iterator for empty store
ok 2 should create the workDir
ok 3 should get empty iterator
# async iterator
ok 4 should get key back
ok 5 should get val back
ok 6 should iterate once
# get()
# return null for non existing key
ok 7 should get null for not exist key
# store string key/val
ok 8 should get VAL after set KEY
# store object value
ok 9 should get VAL_OBJ after set KEY
# put()
ok 10 should put VAL for KEY
# count()
ok 11 should get count 0 after init
ok 12 should get count 1 after put
# keys()
ok 13 should get 0 key after init
ok 14 should get back the key
ok 15 should get 1 key after 1 put
# values()
ok 16 should get 0 value after init
ok 17 should get back the value
ok 18 should get 1 value after 1 put

1..18
# tests 18
# pass  18

# ok

@MeirionHughes
Copy link
Member Author

@zixia Nice. thank you. I'll wait on #498 then and do the final commit.

@ralphtheninja
Copy link
Member

@zixia How do you make those "details"? Just type opening and closing tags manually or do you use some sort of tool for it?

@huan
Copy link
Contributor

huan commented Oct 6, 2017

@ralphtheninja Ha, I learned this trick from someone else on Github before...

I guess it's a html5 tag which will be rendered by Github:

<details>
<summary>
See the details:
</summary>
bla...
bla...
bla...
</details>

@ralphtheninja
Copy link
Member

@MeirionHughes Mind rebasing this onto latest master?

@MeirionHughes
Copy link
Member Author

MeirionHughes commented Oct 8, 2017

/sigh... like wack-o-mole these typings...

image

typescript is now pulling up the leveldown iterator method options as the actual IO generic. I'll see if I can fix it.

@vweevers
Copy link
Member

vweevers commented Oct 8, 2017

@MeirionHughes this does sound like we should have continuous integration tests, as @zixia suggested. Or do you think that's overkill and that the typings will become stable soon?

@MeirionHughes
Copy link
Member Author

A separate repo for typescript testing - to be used when anyone PRs to level-x sounds difficult to set up so it gets the right sources.

I think it more feasible to have one repo for most level-x typings, test there, then have each level-x depend on that for development. But this brings issues with package versioning.

As for the current typing and this PR - I thought I'd managed to the get the iterator generic working perfectly, but somehow its now pulling up that extra type union (see above). While its not perfect, it still does default to any for the iterator options (lte, gte, etc...) when you use it at levelup. For the sake of my sanity, it might just be easier to drop the Key/Value generics on the iterator.

maybe @zixia fancies another stab at typings. Get a fresh perspective.

@vweevers
Copy link
Member

vweevers commented Oct 8, 2017

A separate repo for typescript testing - to be used when anyone PRs to level-x sounds difficult to set up so it gets the right sources.

There are tools that can help with this. It's also fine to start off with something that requires manually triggering builds. Esp. because the alternative (all typings in one repo), like you say, is more difficult to maintain with regards to versioning.

For the sake of my sanity, it might just be easier to drop the Key/Value generics on the iterator.
maybe @zixia fancies another stab at typings. Get a fresh perspective.

Sounds like a plan. You could also open an issue for this and tag it with help wanted.

@MeirionHughes
Copy link
Member Author

MeirionHughes commented Oct 24, 2017

Need to make a decision on this because typescript users are having issues. #500

There is one minor imperfection with regard to the inferring of iterator options (typed). If you explicitly type out the LevelUp type manually there is no issue. That said, these generics are insane... I think there are 9 generic types on LevelUp.

An alternative would be to strip the generics entirely, remove support for infering from the given AbstractLevelDown and type LevelUP with a very limited set of very common options, while allowing other (untyped) options to be passed, where applicable.

Personally, while it was * cough * fun to get bend typescript to infer all the option types based off the AbstactLevelDown I suspect a cut-down approach also has merit in order to reduce complexity.

EDIT: Poll locked.

screenshot from 2018-07-08 13-00-13

cc: @gpresland, @zixia @earslap @vweevers @ralphtheninja @juliangruber

@vweevers
Copy link
Member

@MeirionHughes what do you mean by common options? What are those?

@MeirionHughes
Copy link
Member Author

MeirionHughes commented Oct 24, 2017

Any option, or set of options, that is the same across all AbstractLevelDown implementations.

@vweevers
Copy link
Member

I don't feel qualified to vote. Defer to your judgement on this.

@juliangruber
Copy link
Member

I don't feel qualified to vote. Defer to your judgement on this.

me neither. Which is why I'm starting to be quite sure that we shouldn't land typing without a team that maintains them, ie more than one person who is on board and able to commit.

@huan
Copy link
Contributor

huan commented Oct 24, 2017

My two cents is to landing the current typing definition, and then we will be able to maintain it.

I'm currently using the following dependencies for my TypeScript project:

    "encoding-down": "^2.3.1",
    "leveldown": "^2.0.0",
    "levelup": "github:MeirionHughes/levelup",

@juliangruber
Copy link
Member

What though if we need to do a major types change?

For now, we could merge it but say they are unstable and might break without a major release.

@huan
Copy link
Contributor

huan commented Oct 24, 2017

@juliangruber I'll be ok with "the typings are unstable and might break without a major release" because:

  1. it will only affect the TS developer
  2. it's better than nothing
  3. it will not affect the JS developer at all.

@juliangruber
Copy link
Member

it will only affect the TS developer

It would be great to make TS developers real first class citizens for leveldb, and that's why we need to get the integration right from the start.

Is everyone ok with following a stability index as nodejs does for the typings, and mark them experimental at first?

@ralphtheninja
Copy link
Member

Is everyone ok with following a stability index as nodejs does for the typings, and mark them experimental at first?

Sounds good to me!

@MeirionHughes
Copy link
Member Author

MeirionHughes commented Oct 30, 2017

@sandersn Thanks for the fixes on abstract-leveldown; would welcome any thoughts you had on the levelup typings here. The general idea was for levelup to infer option types from the supplied abstract-leveldown implementation. But as you can see the generics are somewhat complicated. There is a poll above too.

@vweevers
Copy link
Member

vweevers commented Dec 5, 2017

What needs to happen here? Add a note to the readme about the typing stability, rebase, and merge?

@MeirionHughes

@vweevers vweevers mentioned this pull request Dec 5, 2017
@MeirionHughes
Copy link
Member Author

MeirionHughes commented Dec 5, 2017

@vweevers yeah probably; its been stalled because there is no right answer at the moment; the complexity of typing the high-level levelup to whatever low-level abstract-down is given to it just makes this difficult the pros and cons to any approach. That problem simply goes away entirely in the even higher level project though. Even the poll doesn't help really as its almost split equally. :D I'm probably not going to be using this for another 3/4 months as I've been distracted on another project for the last few months. I'd lean to just getting it in as is, IFF @zixia is happy with it still as he/they has used it more.

In any case, as far as I'm aware the current typings available from defiantly-typed are still out-of-date: http://definitelytyped.org/docs/levelup--levelup/modules/levelup.html

@vweevers
Copy link
Member

vweevers commented Dec 6, 2017

Then let's get it in as-is. Perhaps usage in the wild will lead to more feedback.

Do you have the time or do you want someone to take over?

@MeirionHughes
Copy link
Member Author

@vweevers yes, if possible. Spinning too many plates at the moment to give this the attention it deserves.

@vweevers vweevers added the help wanted Extra attention is needed label Dec 6, 2017
@huan
Copy link
Contributor

huan commented Dec 6, 2017

@MeirionHughes @vweevers I vote for getting it in as-it-is because as @MeirionHughes said, I'm happy with it. :-p

@vweevers
Copy link
Member

vweevers commented Dec 6, 2017

Is everyone ok with following a stability index as nodejs does for the typings, and mark them experimental at first?

So how do we achieve this?

EDIT: Poll locked.

screenshot from 2018-07-08 13-01-22

Examples:

experimental experimental

@sqwk
Copy link

sqwk commented Jan 7, 2018

Is there something inherently wrong with the types that is keeping this from being merged? Been using MeirionHughes' repo for the last month and had no problems so far.

@huan
Copy link
Contributor

huan commented Jan 7, 2018

I'd like to suggest to merge the types this month instead of using the repo from MeirionHughes.

@Level/typescript

@MeirionHughes
Copy link
Member Author

MeirionHughes commented Feb 2, 2018

I'll review this as soon as I can and hopefully pull the trigger.

[edit] I tried to get my head into it on the weekend, but with kids running around behind me during the day its difficult. 👪

@MeirionHughes
Copy link
Member Author

MeirionHughes commented Feb 8, 2018

An insane alternative to using generics...

type unknown = {} | undefined | null;

interface AbstractOptions {
  GO: unknown;
  K: unknown;
  V: unknown;
  GET: unknown;
}

interface AbstractLevelDown extends AbstractOptions {
  GO: {
    keyAsBuffer?: boolean;
    valueAsBuffer?: boolean;
  },
  K: string | Buffer;
  V: string | Buffer;
}

interface CodecEncoder<T=unknown> {
  T: T;
  encode: (val: T) => Buffer;
  decode: (val: Buffer) => T;
  buffer: boolean
  type: string
}

interface Codec<K=unknown, V=unknown> {
  K: K;
  V: V;
  keyEncoding?: CodecEncoder<K>
  valueEncoding?: CodecEncoder<V>
}


interface EncodingDown<O extends AbstractOptions, C extends Codec> extends AbstractOptions {
  C: C;
  K: C["K"];
  V: C["V"];

  GO: O["GO"] & {
    valueAsBuffer?: boolean,
    valueEncoding?: CodecEncoder
  }

  GET(key: this["C"]["K"], cb: (value: this["C"]["V"]) => void);
  GET(key: this["C"]["K"], options: this["GO"] & { valueAsBuffer: false }, cb: (value: this["C"]["V"]) => void);
  GET(key: this["C"]["K"], options: this["GO"] & { valueAsBuffer: true }, cb: (value: Buffer) => void);
  GET<VE extends CodecEncoder>(key: this["C"]["K"], options: this["GO"] | { valueEncoding: VE }, cb: (value: VE["T"]) => void);
}

interface LevelUp<AD extends AbstractOptions> {
  GO: AD["GO"];
  K: AD["K"];
  V: AD["V"];

  GET(key: this["K"]): Promise<this["V"]>;
  GET<V = this["V"]>(key: this["K"], options: this["GO"]): Promise<V>;

  get: AD["GET"] & this["GET"];
}


declare function encoding<O extends AbstractOptions, C extends Codec>(db: O, c: C): EncodingDown<O, C>
declare function levelup<O extends AbstractOptions>(db: O): Pick<LevelUp<O>, "get">

type MyKey = {
  foo: string;
}
type MyValue = {
  bar: string;
}
type AltValue = {
  ray: string;
}

let codex = {} as Codec<MyKey, MyValue>
let encoder = encoding({} as AbstractLevelDown, codex);
let db = levelup(encoder);

async function main() {

  const alt = {} as CodecEncoder<AltValue>;

  db.get({ foo: "hello" }, (v) => {
    // v is MyValue

  });
  db.get({ foo: "hello" }, { valueEncoding: alt }, (v) => {
    // v is AltValue

  });
  db.get({ foo: "hello" }, { valueAsBuffer: true }, (v) => {
    // v is Buffer
  });

  let a = await db.get({ foo: "hello" }); // a is MyValue
  let b = await db.get({ foo: "hello" }, { valueEncoding: alt }); // b is still MyValue :(
  let c = await db.get<AltValue>({ foo: "hello" }, { valueEncoding: alt }); // c is AltValue

}

... yeah

@MeirionHughes
Copy link
Member Author

MeirionHughes commented Feb 13, 2018

Some good news at least is that type inference is landing in typescript 2.8 ( missed this as I thought we'd only be getting conditional types). microsoft/TypeScript#21496

So in typescript 2.8 we should hopefully be able to just infer options and return types from the enclosed db and avoid the complex generics.

[Update]: I tested it out; unfortunately it falls short because the conditional-mapping + inference doesn't support overloaded functions. So, for example, Promisifying the get function of the underlaying db won't work just yet because there are at least two different signatures + the ...asBuffer:true signatures. Shame to... as if that worked we could remove all the option generics. :/

@Tapppi
Copy link

Tapppi commented Feb 19, 2018

Figuring out what to do for level typings was a fricking pain, having to muddle through the different repos and issues. This should really get merged, if only to make it clear that there ARE working typings and that they are not stable.. I couldn't find working typings for the promisified version published.

@vweevers
Copy link
Member

We're considering moving the typings out of Level and into DefinitelyTyped. Please see Level/community#16.

@vweevers vweevers mentioned this pull request Jul 8, 2018
6 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants