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

Add and fix TypeScript tests #195

Merged
merged 7 commits into from
Feb 8, 2018
Merged

Add and fix TypeScript tests #195

merged 7 commits into from
Feb 8, 2018

Conversation

vweevers
Copy link
Member

@vweevers vweevers commented Jan 22, 2018

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

@vweevers
Copy link
Member Author

(We need more @Level/typescript people)

self._nexting = false
callback()
callback.apply(null, arguments)
Copy link
Member

Choose a reason for hiding this comment

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

Nice one. Much more cleaner now.

@vweevers
Copy link
Member Author

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

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 we add a test.ts to test TypeScript more natively.

@vweevers
Copy link
Member Author

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
Copy link
Contributor

huan commented Jan 23, 2018

@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
Copy link
Member Author

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 (b1d006e 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 vweevers self-assigned this Jan 23, 2018
@vweevers
Copy link
Member Author

vweevers commented Jan 23, 2018

Remaining tasks:

  • upgrade ts-node
  • add stability badge to readme
  • add simple test.ts, run it in addition to test.js

@vweevers vweevers added the wip label Jan 23, 2018
@vweevers
Copy link
Member Author

@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
Copy link
Member Author

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

@vweevers vweevers removed the wip label Jan 28, 2018
const buf = Buffer.from('foo')

// Key and value generics
new AbstractLevelDOWN<string, string>('loc').put('key', 'value', (err: Error) => {})
Copy link
Member Author

Choose a reason for hiding this comment

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

Note this isn't that valuable, as it's only positive tests, no negative tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also I initially created this file as test.ts but this tripped up TypeScript's module resolution (because we also have test.js). So I renamed it to test-types.ts.

@vweevers
Copy link
Member Author

PTAL

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.

Very good start to have native typescript tests!

@MeirionHughes MeirionHughes requested review from MeirionHughes and removed request for MeirionHughes January 29, 2018 14:46
Copy link
Member

@MeirionHughes MeirionHughes left a comment

Choose a reason for hiding this comment

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

looks fine

@MeirionHughes
Copy link
Member

MeirionHughes commented Jan 29, 2018

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
Copy link
Member Author

vweevers commented Feb 8, 2018

@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.

* remove approximateSize() and isLevelDOWN()
* make Batch value optional
* add missing db property to AbstractIterator
Otherwise TypeScript doesn't see that Test inherits
AbstractLevelDOWN, if defined like this:

```
function Test (location) {
  AbstractLevelDOWN.call(this, location)
}

util.inherits(Test, AbstractLevelDOWN)
```

And will complain about AbstractLevelDOWN methods being undefined on
Test.
@vweevers
Copy link
Member Author

vweevers commented Feb 8, 2018

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

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.

4 participants