Level / abstract-leveldown

An abstract prototype matching the leveldown API.
MIT License
146 stars 53 forks source link

Add and fix TypeScript tests #195

Closed vweevers closed 6 years ago

vweevers commented 6 years ago

Closes #190, #192, #193 (with a few leftover tasks I'll open issues for).

vweevers commented 6 years ago

(We need more @Level/typescript people)

vweevers commented 6 years ago

I'm thinking we should mark the typings as stability: experimental here too.

vweevers commented 6 years ago

I'd like to suggest we add a test.ts to test TypeScript more natively.

@zixia what are the benefits? And does it involve rewriting the test suite in TypeScript?

huan commented 6 years ago

@vweevers The benefits would be able to do static type checking? I do not know whether ts-node do type checking when we run a .js file.

After ts-node v4, we have to do a ts-node --type-check because the default behavior had been changed.

And does it involve rewriting the test suite in TypeScript?

I think we can just add one unit test file written by TypeScript, and we use ts-node --type-check test.ts to run the test under TypeScript with static type checkings.

vweevers commented 6 years ago

The benefits would be able to do static type checking? I do not know whether ts-node do type checking when we run a .js file.

It does (though to a lesser extent, I'm assuming), which is why we need most commits here (b1d006eab4f0665b471b407ac93d62e60757442b for example).

After ts-node v4, we have to do a ts-node --type-check because the default behavior had been changed.

Ah! I was wondering why v4 didn't work. Great, I'll upgrade ts-node to v4.

vweevers commented 6 years ago

Remaining tasks:

vweevers commented 6 years ago

@zixia is there a way to test the types themselves? For example, this should be a failing test:

const db = new AbstractLevelDOWN<string, number>('foobar')

db.put('key', 'value', (err) => {
  t.ifError(err)
  t.end()
})

While this one should pass:

const db = new AbstractLevelDOWN<string, number>('foobar')

db.put('key', 2, (err) => {
  t.ifError(err)
  t.end()
})

Without tests like this, I don't know how to make test.ts valuable.

vweevers commented 6 years ago

Or in other words: how can we test that the type definitions are correct?

vweevers commented 6 years ago

PTAL

MeirionHughes commented 6 years ago

one thing I just couldn't get right was the asBuffer or keyAsBuffer option stuff.

i.e. when K=string and options={keyAsBuffer:true} then the typing for the key on

get(key: K, options: GO, callback: (err: any, value: V) => any): void;

is wrong. Some other solutions I tried, but didn't really work when it came to inference at the levelup stage:

manually override when needed:

get<KK=K>(key: KK, options: GO, callback: (err: any, value: V) => any): void;

overloading:

get(key: K, options: GO, callback: (err: any, value: V) => any): void;
get(key: Buffer, options: GO & {keyAsBuffer:true}, callback: (err: any, value: V) => any): void;

I bring it up because that case wasn't tested in this PR.

This is one reason why I put the idea that the generics be removed in levelup (and potentially everywhere else) and then put in all key/value typing and option overloading in the high-level stuff; like level where it can be explicit

vweevers commented 6 years ago

@MeirionHughes Considering the dynamic nature of everything put together (abstract-leveldown, *asBuffer options, stores, serialization, encoding-down, sublevels, per-operation encodings, etc), using any instead of generics does sound like a good idea to me.

It would simplify integration, decrease coupling, make it easier to maintain for the non-TS-savvy, and we could also increase correctness of the rest of the typings (for example, the *asBuffer options should theoretically be defined here, not in implementations, as it is abstract-leveldown that defines these options and their default values).

That said, if you can find a way to make generics work, then great, go for it ;) Perhaps continue the discussion in a new ticket with @Level/typescript.

As for this PR, I'm gonna squash a few commits and then we're good to merge. I'm not really happy with having to use class in tests, as we don't use classes anywhere else, but it's an acceptable trade-off IMO.

vweevers commented 6 years ago

@ralphtheninja I'll update the changelog in a separate PR.