Fix typings for Batch

[Tapppi]

Tried using the typings with typescript 2.6/2.7 and neither worked. Both throw on type Batch<K=any, V?=any>. b353fb4 added the ? before the default for some reason. AFAIK this is not valid typescript, and servers no purpose. Since type Batch<K=any, V=any> already has defaults, the values are optional. Without the question mark it seems to be working.

@vweevers can you elaborate on what you were trying to achieve?

[vweevers]

Without it, npm run test-ts failed for me. I don't recall why, and it's best if someone with TS knowledge takes a look at this. @Level/typescript

[Tapppi]

Without it, npm run test-ts failed for me. I don't recall why, and it's best if someone with TS knowledge takes a look at this

Yep, noticed, I'll try to see what's up but it's a pretty mystic failure..

[Tapppi]

Okay, figured out what is wrong.. The type for Batch:

export type Batch<K=any, V=any> = PutBatch<K, V> | DelBatch<K>

export interface PutBatch<K=any, V=any> {
  type: 'put',
  key: K,
  value: V
}

export interface DelBatch<K=any> {
  type: 'del',
  key: K
}

The test input that results in error, even though there should be nothing wrong:

var expectedArray: {}[] = [
    { type: 'put', key: '1', value: '1' },
    { type: 'del', key: '2' }
  ]
// Should be compatible with Batch<any, any>[], but produces error:
test.js (244,14): Argument of type '({ [x: string]: any; type: string; key: string; value: string; } | { [x: string]: any; type: stri...' is not assignable to parameter of type 'Batch<any, any>[]'.
  Type '{ [x: string]: any; type: string; key: string; value: string; } | { [x: string]: any; type: strin...' is not assignable to type 'Batch<any, any>'.
    Type '{ [x: string]: any; type: string; key: string; value: string; }' is not assignable to type 'Batch<any, any>'.
      Type '{ [x: string]: any; type: string; key: string; value: string; }' is not assignable to type 'DelBatch<any>'.
        Types of property 'type' are incompatible.
          Type 'string' is not assignable to type '"del"'. (2345)

This is something I did hit in my own code at one point when using discriminated unions. For some reason the object { type: 'del', key: '2' } is getting inferred as { ... ,type: 'string, ...} which does not pass the type check. @Level/typescript if anyone knows how to deal with this in a nice way, I would be interested. But as it stands, this is a problem in inferring the test types and the current typings in master are plain invalid.

@vweevers Why didn't the tests turn red for invalid typings? If the typings file doesn't compile at all, it is ignored instead of throwing an error?

[vweevers]

Thanks for looking into this!

var expectedArray = [
  { type: 'put', key: '1', value: '1' },
  { type: 'del', key: '2' }
]

Is there a way to annotate JS like this, perhaps with JSDoc, to cast it to the right type? So that we can get our tests passing and land this PR.

[vweevers]

@vweevers Why didn't the tests turn red for invalid typings? If the typings file doesn't compile at all, it is ignored instead of throwing an error?

That would be bad. TypeScript not barking led me to believe it was valid (and that it did compile). If you're right about TS ignoring the typings altogether, I hope there's a way to disable that behavior?

It was also my mistake, thinking I could do TS. Would you be interested in helping out to maintain the typings?

[Tapppi]

It was also my mistake, thinking I could do TS. Would you be interested in helping out to maintain the typings?

I'm fairly new to the world of TS myself, but am invested in it as I'll be using TS and level-rocksdb for a major Kafka Streams TS project. My time is fairly contested right now, but should be able to try and help with the typings. I'm taking another stab at this as I think I'll be needing this solved for my own projects as well..

[Tapppi]

If I put this clearly invalid typescript into the typings, all npm run test-ts tests run fine.

asfdasdf
export ???ASDFASDF???? type Batch<K=any, V?=any> = PutBatch<K, V> | DelBatch<K>

If I fix the typings, the tests bark at the invalid typecheck. This is fun 😄

Apparently ts-node doesn't actually check that it gets types, but ignores the malformed file. It just checks types when it gets the correct typings and then reacts to the type error. tsc -p . (Typescript compiler) does bark at the malformed typings.

If you're right about TS ignoring the typings altogether, I hope there's a way to disable that behavior?

Don't see a way of disabling this behaviour in ts-node.

Is there a way to annotate JS like this, perhaps with JSDoc, to cast it to the right type? So that we can get our tests passing and land this PR.

Test file isn't TS so there isn't a way to use typings without using .ts extension (at least to my knowledge?).

If I convert to test.ts, I can fix the error by typing the batch array explicitly:

var expectedArray: {type: 'put' | 'del', key: string, value?: string}[] = [
    { type: 'put', key: '1', value: '1' },
    { type: 'del', key: '2' }
  ]
// or just:
var expectedArray: Batch[] = [
    { type: 'put', key: '1', value: '1' },
    { type: 'del', key: '2' }
  ]

But theres a ton of typescript compiler errors in test.ts, it's pretty loosely (javascriptly) written..

---

With some mangling of config and test.ts, there's only one error I can't seem to fix.

class Test extends AbstractLevelDOWN {}
Test.prototype._del = spy

test = new Test('foobar') // This throws

The original typings set AbstractLevelDOWN: AbstractLevelDOWNConstructor. With tsc/ts-node and this code I get:
test.ts(450,14): error TS2554: Expected 0 arguments, but got 1.

If I replace the constructor typing with class declaration in index.d.ts, I get different outputs for tsc/ts-node:

ts-node: test-types.ts (17,1): Cannot use 'new' with an expression whose type lacks a call or construct signature. (2351)

tsc: test.ts(450,14): error TS2554: Expected 0 arguments, but got 1.

Here is the class declaration and export I tried:

declare class AbstractLevelDOWNA<K=any, V=any, O=any, PO=any, GO=any, DO=any, IO=any, BO=any> {
    constructor(location: string);

    open(callback: (err?: any) => void): void;
    open(options: O, callback: (err?: any) => void): void;

    close(callback: (err?: any) => void): void;

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

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

    del(key: K, callback: (err: any) => any): void;
    del(key: K, options: DO, callback: (err: any) => any): void;

    batch(): AbstractChainedBatch<K, V, BO>;
    batch(array: Batch<K, V>[], callback: (err: any) => any): AbstractChainedBatch<K, V, BO>;
    batch(array: Batch<K, V>[], options: BO, callback: (err: any) => any): AbstractChainedBatch<K, V, BO>;

    iterator(options?: IO & AbstractIteratorOptions<K>): AbstractIterator<K, V>;

    [index: string]: any;
}
export const AbstractLevelDOWN: AbstractLevelDOWNA;

I'm starting to think there is something really wrong with having a constructor and using an extends on it somehow.. Or this is done really wrongly somehow subtly, or this is a typescript bug. I'm kind of lost at this point.

[huan]

I had cloned the @Tapppi version of this PR in my Ubuntu box, and it passed all the tests by npm i && npm t without any problem.

Wired.

My Log

┌ zixia@zixia-desktop:~/git/abstract-leveldown [18:15:23] tty:[2] jobs:[0]
└ {master} $ ./node_modules/.bin/ts-node --type-check --no-cache test.js
TAP version 13
# test database creation no-arg throws
ok 1 no-arg leveldown() throws
# test database creation non-string location throws
ok 2 non-string location leveldown() throws
# test database open no-arg throws
ok 3 database object returned
ok 4 open() function exists
# test database open no-arg throws
ok 5 no-arg open() throws
# test callback-less, 1-arg, open() throws
ok 6 callback-less, 1-arg open() throws
# setUp common
ok 7 cleanup returned an error
# setUp db
# test argument-less del() throws
ok 8 no-arg del() throws
# test callback-less, 1-arg, del() throws
ok 9 callback-less, 1-arg del() throws
# test callback-less, 3-arg, del() throws
ok 10 callback-less, 2-arg del() throws
# test custom _serialize*
ok 11 should be equivalent
ok 12 undefined
ok 13 undefined
# setUp common
ok 14 cleanup returned an error
# setUp db
# test argument-less get() throws
ok 15 no-arg get() throws
# test callback-less, 1-arg, get() throws
ok 16 callback-less, 1-arg get() throws
# test callback-less, 3-arg, get() throws
ok 17 callback-less, 2-arg get() throws
# test custom _serialize*
ok 18 should be equivalent
ok 19 undefined
ok 20 undefined
# setUp common
ok 21 cleanup returned an error
# setUp db
# test argument-less put() throws
ok 22 no-arg put() throws
# test callback-less, 1-arg, put() throws
ok 23 callback-less, 1-arg put() throws
# test callback-less, 2-arg, put() throws
ok 24 callback-less, 2-arg put() throws
# test callback-less, 3-arg, put() throws
ok 25 callback-less, 3-arg put() throws
# test _serialize object
ok 26 should be truthy
ok 27 should be truthy
ok 28 undefined
# test custom _serialize*
ok 29 should be equivalent
ok 30 should be equivalent
ok 31 undefined
ok 32 undefined
# setUp common
ok 33 cleanup returned an error
# setUp db
# test get() with null key causes error
ok 34 has error
ok 35 should be truthy
ok 36 correct error message
ok 37 callback is asynchronous
# test del() with null key causes error
ok 38 has error
ok 39 should be truthy
ok 40 correct error message
ok 41 callback is asynchronous
# test put() with null key causes error
ok 42 has error
ok 43 should be truthy
ok 44 correct error message
ok 45 callback is asynchronous
# test get() with undefined key causes error
ok 46 has error
ok 47 should be truthy
ok 48 correct error message
ok 49 callback is asynchronous
# test del() with undefined key causes error
ok 50 has error
ok 51 should be truthy
ok 52 correct error message
ok 53 callback is asynchronous
# test put() with undefined key causes error
ok 54 has error
ok 55 should be truthy
ok 56 correct error message
ok 57 callback is asynchronous
# test get() with empty String key causes error
ok 58 has error
ok 59 should be truthy
ok 60 correct error message
ok 61 callback is asynchronous
# test del() with empty String key causes error
ok 62 has error
ok 63 should be truthy
ok 64 correct error message
ok 65 callback is asynchronous
# test put() with empty String key causes error
ok 66 has error
ok 67 should be truthy
ok 68 correct error message
ok 69 callback is asynchronous
# test get() with empty Buffer key causes error
ok 70 has error
ok 71 should be truthy
ok 72 correct error message
ok 73 callback is asynchronous
# test del() with empty Buffer key causes error
ok 74 has error
ok 75 should be truthy
ok 76 correct error message
ok 77 callback is asynchronous
# test put() with empty Buffer key causes error
ok 78 has error
ok 79 should be truthy
ok 80 correct error message
ok 81 callback is asynchronous
# test get() with empty Array key causes error
ok 82 has error
ok 83 should be truthy
ok 84 correct error message
ok 85 callback is asynchronous
# test del() with empty Array key causes error
ok 86 has error
ok 87 should be truthy
ok 88 correct error message
ok 89 callback is asynchronous
# test put() with empty Array key causes error
ok 90 has error
ok 91 should be truthy
ok 92 correct error message
ok 93 callback is asynchronous
# tearDown
ok 94 cleanup returned an error
# setUp common
ok 95 cleanup returned an error
# setUp db
# test callback-less, 2-arg, batch() throws
ok 96 callback-less, 2-arg batch() throws
# test batch() with missing `value`
ok 97 undefined
# test batch() with null `value`
ok 98 undefined
# test batch() with missing `key`
ok 99 got error
ok 100 correct error message
ok 101 callback is asynchronous
# test batch() with null `key`
ok 102 got error
ok 103 correct error message
ok 104 callback is asynchronous
# test batch() with missing `key` and `value`
ok 105 got error
ok 106 correct error message
ok 107 callback is asynchronous
# test batch() with missing `type`
ok 108 got error
ok 109 correct error message
ok 110 callback is asynchronous
# test batch() with wrong `type`
ok 111 got error
ok 112 correct error message
ok 113 callback is asynchronous
# test batch() with missing array
ok 114 got error
ok 115 correct error message
ok 116 callback is asynchronous
# test batch() with undefined array
ok 117 got error
ok 118 correct error message
ok 119 callback is asynchronous
# test batch() with null array
ok 120 got error
ok 121 correct error message
ok 122 callback is asynchronous
# test batch() with null options
ok 123 undefined
# test batch() with null element
ok 124 got error
ok 125 correct error message
ok 126 callback is asynchronous
# test batch() with undefined element
ok 127 got error
ok 128 correct error message
ok 129 callback is asynchronous
# test batch() with number element
ok 130 got error
ok 131 correct error message
ok 132 callback is asynchronous
# test batch() with boolean element
ok 133 got error
ok 134 correct error message
ok 135 callback is asynchronous
# setUp common
ok 136 cleanup returned an error
# setUp db
# test batch#put() with missing `value`
# test batch#put() with null `value`
# test batch#put() with missing `key`
ok 137 correct error message
# test batch#put() with null `key`
ok 138 correct error message
# test batch#put() with missing `key` and `value`
ok 139 correct error message
# test batch#del() with missing `key`
ok 140 correct error message
# test batch#del() with null `key`
ok 141 correct error message
# test batch#del() with null `key`
ok 142 correct error message
# test batch#clear() doesn't throw
# test batch#write() with no callback
ok 143 correct error message
# test batch#put() after write()
ok 144 correct error message
# test batch#del() after write()
ok 145 correct error message
# test batch#clear() after write()
ok 146 correct error message
# test batch#write() after write()
ok 147 correct error message
# test serialize object
ok 148 .key is set for .put and .del operations
ok 149 .value is set for .put operation
ok 150 .key is set for .put and .del operations
# test custom _serialize*
ok 151 should be equivalent
# test close()
ok 152 undefined
ok 153 no-arg close() throws
ok 154 non-callback close() throws
ok 155 undefined
# setUp common
ok 156 cleanup returned an error
# setUp db
# test argument-less iterator#next() throws
ok 157 no-arg iterator#next() throws
# test argument-less iterator#end() after next() throws
ok 158 no-arg iterator#end() throws
# test argument-less iterator#end() throws
ok 159 no-arg iterator#end() throws
# test iterator#next returns this
ok 160 should be truthy
# test twice iterator#end() callback with error
ok 161 undefined
ok 162 returned error
ok 163 correct error
ok 164 should be equal
ok 165 callback is asynchronous
# test iterator#next after iterator#end() callback with error
ok 166 undefined
ok 167 returned error
ok 168 correct error
ok 169 correct message
ok 170 callback is asynchronous
# test twice iterator#next() throws
ok 171 undefined
ok 172 returned error
ok 173 correct error
ok 174 should be equal
ok 175 callback is asynchronous
ok 176 undefined
# test core extensibility
ok 177 location set on instance
# test key/value serialization
ok 178 _serializeKey converts to string
ok 179 _serializeKey returns Buffer as is
ok 180 _serializeValue converts null to empty string
ok 181 _serializeValue converts undefined to empty string
ok 182 _serializeValue converts to string
ok 183 _serializeValue returns Buffer as is
ok 184 _serializeValue returns value as is when process.browser
# test open() extensibility
ok 185 got _open() call
ok 186 `this` on _open() was correct
ok 187 got two arguments
ok 188 got default options argument
ok 189 got _open() call
ok 190 `this` on _open() was correct
ok 191 got two arguments
ok 192 got expected options argument
# test close() extensibility
ok 193 got _close() call
ok 194 `this` on _close() was correct
ok 195 got one arguments
# test get() extensibility
ok 196 got _get() call
ok 197 `this` on _get() was correct
ok 198 got three arguments
ok 199 got expected key argument
ok 200 got default options argument
ok 201 got expected cb argument
ok 202 got _get() call
ok 203 `this` on _get() was correct
ok 204 got three arguments
ok 205 got expected key argument
ok 206 got expected options argument
ok 207 got expected cb argument
# test del() extensibility
ok 208 got _del() call
ok 209 `this` on _del() was correct
ok 210 got three arguments
ok 211 got expected key argument
ok 212 got blank options argument
ok 213 got expected cb argument
ok 214 got _del() call
ok 215 `this` on _del() was correct
ok 216 got three arguments
ok 217 got expected key argument
ok 218 got expected options argument
ok 219 got expected cb argument
# test put() extensibility
ok 220 got _put() call
ok 221 `this` on _put() was correct
ok 222 got four arguments
ok 223 got expected key argument
ok 224 got expected value argument
ok 225 got blank options argument
ok 226 got expected cb argument
ok 227 got _put() call
ok 228 `this` on _put() was correct
ok 229 got four arguments
ok 230 got expected key argument
ok 231 got expected value argument
ok 232 got blank options argument
ok 233 got expected cb argument
# test batch() extensibility
ok 234 got _batch() call
ok 235 `this` on _batch() was correct
ok 236 got three arguments
ok 237 got expected array argument
ok 238 got expected options argument
ok 239 got expected callback argument
ok 240 got _batch() call
ok 241 `this` on _batch() was correct
ok 242 got three arguments
ok 243 got expected array argument
ok 244 got expected options argument
ok 245 got expected callback argument
ok 246 got _batch() call
ok 247 `this` on _batch() was correct
ok 248 got three arguments
ok 249 got expected array argument
ok 250 options should not be null
ok 251 got expected callback argument
# test chained batch() (array) extensibility
ok 252 got _batch() call
ok 253 `this` on _batch() was correct
ok 254 got three arguments
ok 255 got expected array argument
ok 256 got expected array argument[0]
ok 257 got expected array argument[1]
ok 258 got expected options argument
ok 259 got expected callback argument
ok 260 got _batch() call
ok 261 `this` on _batch() was correct
ok 262 got three arguments
ok 263 got expected array argument
ok 264 got expected array argument[0]
ok 265 got expected array argument[1]
ok 266 got expected options argument
ok 267 got expected callback argument
# test chained batch() (custom _chainedBatch) extensibility
ok 268 got _chainedBatch() call
ok 269 `this` on _chainedBatch() was correct
ok 270 got _chainedBatch() call
ok 271 `this` on _chainedBatch() was correct
# test AbstractChainedBatch extensibility
ok 272 db set on instance
# test write() extensibility
ok 273 got _write() call
ok 274 `this` on _write() was correct
ok 275 got one argument
ok 276 got a callback function
ok 277 spycb not called
ok 278 spycb called, i.e. was our cb argument
# test put() extensibility
ok 279 got _put call
ok 280 `this` on _put() was correct
ok 281 got two arguments
ok 282 got expected key argument
ok 283 got expected value argument
ok 284 get expected return value
# test del() extensibility
ok 285 got _del call
ok 286 `this` on _del() was correct
ok 287 got one argument
ok 288 got expected key argument
ok 289 get expected return value
# test clear() extensibility
ok 290 got _clear call
ok 291 `this` on _clear() was correct
ok 292 got zero arguments
ok 293 get expected return value
# test iterator() extensibility
ok 294 got _close() call
ok 295 `this` on _close() was correct
ok 296 got one arguments
ok 297 got expected options argument
# test AbstractIterator extensibility
ok 298 db set on instance
# test next() extensibility
ok 299 got _next() call
ok 300 `this` on _next() was correct
ok 301 got one arguments
ok 302 got a callback function
ok 303 spycb not called
ok 304 spycb called, i.e. was our cb argument
# test end() extensibility
ok 305 got _end() call
ok 306 `this` on _end() was correct
ok 307 got one arguments
ok 308 got expected cb argument
# test serialization extensibility (put)
ok 309 should be equal
ok 310 should be equal
ok 311 got _put() call
ok 312 got expected key argument
ok 313 got expected value argument
# test serialization extensibility (del)
ok 314 should be equal
ok 315 got _del() call
ok 316 got expected key argument
# test serialization extensibility (batch array put)
ok 317 should be equal
ok 318 should be equal
ok 319 got _batch() call
ok 320 got expected key
ok 321 got expected value
# test serialization extensibility (batch chain put)
ok 322 should be equal
ok 323 should be equal
ok 324 got _batch() call
ok 325 got expected key
ok 326 got expected value
# test serialization extensibility (batch array del)
ok 327 should be equal
ok 328 got _batch() call
ok 329 got expected key
# test serialization extensibility (batch chain del)
ok 330 should be equal
ok 331 got _batch() call
ok 332 got expected key
# test serialization extensibility (batch array is not mutated)
ok 333 should be equal
ok 334 should be equal
ok 335 got _batch() call
ok 336 got expected key
ok 337 got expected value
ok 338 did not mutate input key
ok 339 did not mutate input value
# .status
# empty prototype
ok 340 should be equal
ok 341 undefined
ok 342 should be equal
ok 343 undefined
ok 344 should be equal
# open error
ok 345 should be truthy
ok 346 should be equal
# close error
ok 347 should be truthy
ok 348 should be equal
# open
ok 349 should be equal
ok 350 undefined
ok 351 should be equal
# close
ok 352 undefined
ok 353 should be equal
ok 354 undefined
ok 355 should be equal
# _setupIteratorOptions
# default options
ok 356 correct defaults
# set options
ok 357 options set correctly
# deletes empty buffers
ok 358 should be buffer
ok 359 should be empty
ok 360 should be buffer
ok 361 should be empty
ok 362 should be buffer
ok 363 should be empty
ok 364 should be buffer
ok 365 should be empty
ok 366 should be buffer
ok 367 should be empty
ok 368 should be buffer
ok 369 should be empty
ok 370 property should be deleted
ok 371 property should be deleted
ok 372 property should be deleted
ok 373 property should be deleted
ok 374 property should be deleted
ok 375 property should be deleted
# deletes empty strings
ok 376 should be string
ok 377 should be empty
ok 378 should be string
ok 379 should be empty
ok 380 should be string
ok 381 should be empty
ok 382 should be string
ok 383 should be empty
ok 384 should be string
ok 385 should be empty
ok 386 should be string
ok 387 should be empty
ok 388 property should be deleted
ok 389 property should be deleted
ok 390 property should be deleted
ok 391 property should be deleted
ok 392 property should be deleted
ok 393 property should be deleted
# deletes null options
ok 394 should be null
ok 395 should be null
ok 396 should be null
ok 397 should be null
ok 398 should be null
ok 399 should be null
ok 400 property should be deleted
ok 401 property should be deleted
ok 402 property should be deleted
ok 403 property should be deleted
ok 404 property should be deleted
ok 405 property should be deleted

1..405
# tests 405
# pass  405

# ok

[Tapppi]

I'm thinking this is some weird problem with ts-node. If I run npm run test-ts I still get the errors with this branch..

I got all of the problems fixed by converting the test file to typescript (test.ts), explicitly setting all constructor types and the Batch[] typing. Then used tsc -p . to compile and type check. Not sure what the consensus would be on doing that.. Also don't know if the explicit constructor types is something that will bug out with dependant libraries as well. Might have to test that out.

@zixia can you please try renaming the test file to test.ts and running:

[yarn|npx] ts-node -v
[yarn|npx] tsc test.ts index.d.ts global.d.ts test-types.ts

// My versions:
ts-node v4.1.0
node v6.12.2
typescript v2.7.2

Tried with node 8 too, but that made no difference. If we still get diverging behaviour this is really weird. If we don't it's probably ts-node fucking things up.

[huan]

$ ./node_modules/.bin/ts-node -v
ts-node v4.1.0
node v9.5.0
typescript v2.7.2

$ ./node_modules/.bin/tsc test.ts index.d.ts global.d.ts test-types.ts
test.ts(55,14): error TS2554: Expected 0 arguments, but got 1.
test.ts(64,14): error TS2554: Expected 0 arguments, but got 1.
test.ts(95,10): error TS2554: Expected 0 arguments, but got 1.
test.ts(105,19): error TS2339: Property 'options' does not exist on type '{ createIfMissing: boolean; errorIfExists: boolean; }'.
test.ts(122,10): error TS2554: Expected 0 arguments, but got 1.
test.ts(141,10): error TS2554: Expected 0 arguments, but got 1.
test.ts(153,19): error TS2339: Property 'options' does not exist on type '{ asBuffer: boolean; }'.
test.ts(174,10): error TS2554: Expected 0 arguments, but got 1.
test.ts(206,10): error TS2554: Expected 0 arguments, but got 1.
test.ts(242,10): error TS2554: Expected 0 arguments, but got 1.
test.ts(282,10): error TS2554: Expected 0 arguments, but got 1.
test.ts(316,10): error TS2554: Expected 0 arguments, but got 1.
test.ts(334,14): error TS2554: Expected 0 arguments, but got 1.
test.ts(348,10): error TS2554: Expected 0 arguments, but got 1.
test.ts(373,10): error TS2554: Expected 0 arguments, but got 1.
test.ts(393,10): error TS2554: Expected 0 arguments, but got 1.
test.ts(411,10): error TS2554: Expected 0 arguments, but got 1.
test.ts(436,10): error TS2554: Expected 0 arguments, but got 1.
test.ts(448,14): error TS2554: Expected 0 arguments, but got 1.
test.ts(461,10): error TS2554: Expected 0 arguments, but got 1.
test.ts(483,10): error TS2554: Expected 0 arguments, but got 1.
test.ts(513,10): error TS2554: Expected 0 arguments, but got 1.
test.ts(540,10): error TS2554: Expected 0 arguments, but got 1.
test.ts(569,10): error TS2554: Expected 0 arguments, but got 1.
test.ts(597,10): error TS2554: Expected 0 arguments, but got 1.
test.ts(624,10): error TS2554: Expected 0 arguments, but got 1.
test.ts(650,10): error TS2554: Expected 0 arguments, but got 1.
test.ts(677,10): error TS2554: Expected 0 arguments, but got 1.
test.ts(696,12): error TS2554: Expected 0 arguments, but got 1.
test.ts(720,12): error TS2554: Expected 0 arguments, but got 1.
test.ts(737,12): error TS2554: Expected 0 arguments, but got 1.
test.ts(756,12): error TS2554: Expected 0 arguments, but got 1.
test.ts(774,12): error TS2554: Expected 0 arguments, but got 1.

Aha, I caught the errors...

[huan]

I believe we have to put as any to our mocked data, as the following:

  var expectedArray = [
    { type: 'put', key: '1', value: '1' },
    { type: 'del', key: '2' }
  ] as any

Or better to set it to as Batch<any, any>[]

However, this could not be possible if we name the file test.js, we have to rename the test.js to test.ts to satisfied this change.

[vweevers]

However, this could not be possible if we name the file test.js, we have to rename the test.js to test.ts to satisfied this change.

Perhaps try https://github.com/Microsoft/TypeScript/wiki/JSDoc-support-in-JavaScript to annotate the JS. We have to keep test.js in any case. We can consider adding a secondary test.ts file, but it worries me. We'd need a commitment from you to keep this up to date with test.js. And PRs that add new tests to test.js, wouldn't pass until test.ts is updated. That's problematic.

[vweevers]

And PRs that add new tests to test.js, wouldn't pass until test.ts is updated.

Hm, that won't always be the case. More likely, test.ts slowly becomes outdated.

[huan]

@vweevers What is the reason for this?

We have to keep test.js in any case

I'd like to suggest start using the TypeScript for writing the unit tests because it also could do whatever the .js could do.

[vweevers]

@zixia I can't find it just now but I recall an earlier discussion about keeping the JS. In a nutshell: though we want to make TS a first-class citizen, it does not replace JS. Majority of users and maintainers write in JS.

@ralphtheninja mind weighing in?

[ralphtheninja]

@zixia I can't find it just now but I recall an earlier discussion about keeping the JS. In a nutshell: though we want to make TS a first-class citizen, it does not replace JS. Majority of users and maintainers write in JS.

@ralphtheninja mind weighing in?

I can only underline this. Bringing in typings to aid people using TypeScript is cool, but if it introduces more complexity and problems where the solution is to rewrite things in TypeScript, then I think we're heading in the wrong way.

Introducing more complexity which causes problems that you solve by more complexity just gives me a bad gut feeling. Call me a dinosaur 😄

[huan]

So according to the opinions from @vweevers and @ralphtheninja, I believe the only direction now is to try https://github.com/Microsoft/TypeScript/wiki/JSDoc-support-in-JavaScript to annotate the JS.

@Tapppi Good luck!

[vweevers]

I believe the only direction now is to try https://github.com/Microsoft/TypeScript/wiki/JSDoc-support-in-JavaScript to annotate the JS.

If that doesn't pan out, would removing all the generics - see #195 (comment) and Level/levelup#499 (comment) - also solve this? Or is it unrelated?

[sandersn]

Following @zixia's suggestion, you can also add // @ts-check in test.js to get Typescript to report all the errors once you have the tests type annotated with jsdoc.

(I'm still trying to understand all the discussion in this thread, after which I may have some other suggestions for getting Typescript to co-operate.)

[huan]

@vweevers i think it's unrelated with the generics.

It's a normal type unmatch.

[sandersn]

Here are the jsdocs you need to fix the compile errors:

Line 233, before var expectedArray:
/** @type {Array<{ type: 'put', key, value } | { type: 'del', key }>} */

Line 680, before var op:
/** @type { { type: 'put', key, value } } */

This is the bare minimum you need to get test.js to compile without errors. It is nicer if you import the type Batch. I did this by adding import * as zelf from './'. Then you can refer to the type Batch in jsdoc comments:

/** @type {Array<zelf.Batch>} */

/** @type {zelf.Batch} */

respectively. Unfortunately, Typescript doesn't support the same type-importing from var zelf = require('./') syntax. Fixing that is tracked by bug microsoft/TypeScript#11825

[Tapppi]

@vweevers @zixia

So according to the opinions from @vweevers and @ralphtheninja, I believe the only direction now is to try https://github.com/Microsoft/TypeScript/wiki/JSDoc-support-in-JavaScript to annotate the JS.

That's alright, good to know about the JSDoc support anyway. Also, I agree about converting all of the tests to TS, does not seem like a sensible thing to do in the grander scheme of things, but couldn't hurt to ask 😉

---

@sandersn

This is the bare minimum you need to get test.js to compile without errors. It is nicer if you import the type Batch. I did this by adding import * as zelf from './'. Then you can refer to the type Batch in jsdoc comments

Thanks, I think I'll stick with the plain types, to avoid changing style and unused imports.

Following @zixia's suggestion, you can also add // @ts-check in test.js to get Typescript to report all the errors once you have the tests type annotated with jsdoc.

Thanks! A few questions if you don't mind:

• Is // @ts-check necessary for ts-node --type-check --no-cache test.js, or just when using tsc?
• do you have any idea what's up with the linux/macos ts-node differences? Or is it somehow related to // @ts-check.. If we changed to tsc, it would be nice to only typecheck, not emit files. Don't know if that's possible
• Do you have any idea why type inference fails for the Batch array? Does typescript never try to infer union constant types? I fail to understand why that would be.

[huan]

CI turns green, LGTM

[sandersn]

@Tapppi

1. // @ts-check doesn't make a difference for me, actually, because tsconfig has checkJs: true. I didn't see that until after posting the above comment. I think ts-node is incorrectly swallowing syntax errors, perhaps, because it reports type errors just fine.
2. I am not sure which linux/macos differences you're referring to. Is that information in the original bug? If you want to use tsc for typechecking only, tsc --noEmit (or "noEmit": true in tsconfig) will typecheck without emitting.
3. var x = { a: "hi" } infers a: string unless it has some contextual information that prompts it to expect [immutable] string literals. For a declaration like var (or even const), only a type annotation would supply context, which is why you have to add those in test.js.

Typescript is pretty conservative, so unless it has evidence that something is immutable, it assumes that it is mutable. If the Typescript project were started today, that might be different, since functional style has become more popular.

[Tapppi]

@sandersn

1. Yep, although I was wondering on a general level. Tested it and either tsconfig.json checkJs or //@ts-check IS required for ts-node too.
2. @zixia reported that ts-node was swallowing the type errors for him here, but then got them to throw with tsc here
3. Yeah I get this after rereading typescript mission statements and thinking about it a bit more. Might be an interesting weekend project to try and get TS to not throw for these discriminated unions without changing other behaviour.

[vweevers]

General question, going forward. When would we need additional JSDoc comments, and how do we prevent it from blocking PRs? We can't expect JS contributors to add or even know about JSDoc comments.

[sandersn]

@vweevers After some discussion with @mhegazy, I think the best approach is to start shipping types from DefinitelyTyped and stop shipping them with the package. This allows contributors not to care about Typescript, and Typescript users can easily submit PRs to Definitely Typed if the types get broken.

This does come with a few downsides:

1. The tests and types on DT need to be updated whenever there's a release. It has the same problem as maintaining test.js AND test.ts. Somebody needs to sign up for this to ensure consistent quality.
2. Removing types from the package is a breaking change; typescript users need to know that they will have to add @types/abstract-leveldown to package.json. This should probably wait until a major release.

A few years ago our advice was different because Definitely Typed didn't have nice integration with npm, but now npm install --save-dev @types/abstract-leveldown is reasonably simple to add.

[Tapppi]

@vweevers

General question, going forward. When would we need additional JSDoc comments, and how do we prevent it from blocking PRs?

With current configuration, we'd need them when using typings that TS can't properly infer in tests. Only way to NEVER block PRs because of breaking types is to not typecheck the tests and have more .ts tests. Altogether, it's probably best to do as @sandersn suggested and move the typings to DefinitelyTyped.

Removing types from the package is a breaking change; typescript users need to know that they will have to add @types/abstract-leveldown to package.json. This should probably wait until a major release.

Not necessarily. The typings haven't worked for a while in this package, aren't even included in top-level packages (level-rocksdb, levelup, level etc.) since v2 (work ongoing in Level/levelup#499). I was trying to get typings to work for top-level packages from user side and ended up having to define the typings in my own project files.. So there are two questions:

• Do we maintain the typings and either test.js types or a separate test.ts file in this repo until next major release, when they are removed in favor of DefinitelyTyped?
• Who maintains the DefinitelyTyped typings when there are new releases of *level*?

This should probably also be reflected across Level packages, since this isn't where most users would be using them.

[vweevers]

Before we discuss how to get there (it's not a problem), let's make sure everyone is on board with moving to DefinitelyTyped. Thumbs up? @Level/typescript

[vweevers]

I think we can merge this PR in the mean time, but let's answer the above question as soon as possible.

[vweevers]

@ralphtheninja are you OK with merging this, and releasing a patch? We can then open a new issue for moving to DefinitelyTyped, and give everybody some time to respond.

[ralphtheninja]

@vweevers Lets do it 😄

[vweevers]

4.0.3