Improve typings for t.is and t.deepEqual

[papb]

This PR improves typings for t.is. I created a test to show the difference. In short, the code below will compile fine, while the typings should give an error.

declare function getLiteralStringUnion(): 'hello' | 'world';

test('foo', t => {
  t.is(getLiteralStringUnion(), 'another-string');
});

This is a "workaround" for microsoft/TypeScript#40377 (although it is not settled whether it's really a bug or intended behavior).

[novemberborn]

Thanks for the PR @papb.

This is a "workaround" for microsoft/TypeScript#40377 (although it is not settled whether it's really a bug or intended behavior).

Let's wait for the conclusion to that.

[papb]

Ok :) Why did you change the title to "when using TS 4" though? It also happens on older versions

[novemberborn]

Why did you change the title to "when using TS 4" though? It also happens on older versions

Oh I just got the impression this was a TS 4 thing.

How does this come up in practice? Arguably the test will fail, so I don't mind if TypeScript complains pre-emptively.

[papb]

Oh I just got the impression this was a TS 4 thing.

I re-edited the issue title, I hope you don't mind.

How does this come up in practice?

What do you mean?

Arguably the test will fail, so I don't mind if TypeScript complains pre-emptively.

Precisely, I think the same, the test will definitely fail, so it would be great for TypeScript to complain about it immediately, instead of us having to run the test to see it fail. Also having the autocomplete is immensely useful (in fact the lack of autocomplete in a real situation with me is what made me notice this issue)

[novemberborn]

Oh, right… currently our type definition lets this through, and then the test fails! Sorry I was completely in a "there is a bug" mindset.

This would be a breaking change, no?

[papb]

Oh, right… currently our type definition lets this through, and then the test fails! Sorry I was completely in a "there is a bug" mindset.

No problem, it happens 😅

This would be a breaking change, no?

Hmm, good question. Is a change from "compiling fine and definitely failing at runtime" to "not compiling" a breaking change?

[papb]

On a side note, tests are failing only for Node 14 in Windows... The error seems completely unrelated at a first glance. I tried rerunning but the same failure happened again. Do you have any idea what could be happening? Does AVA read it's own source code to generate error messages, somehow?

Edit: This error did not happen when I first opened the PR, changing only t.is... 🤔

[novemberborn]

Thinking about this some more, we actually have use cases where it's OK to have a failing test (test.failing(), and inside a t.try() assertion). I'm not sure adding a compile time error is all that useful.

And yea I think we'd have to consider it a breaking change.

The test failures are due to a test regression with Node.js 14.9.

[papb]

Hi @novemberborn, the TS issue was closed as working as intended. I still believe this addition would be useful though.

Thinking about this some more, we actually have use cases where it's OK to have a failing test (test.failing(), and inside a t.try() assertion). I'm not sure adding a compile time error is all that useful.

Interesting, I have never used those... However, note that there is already a typechecking limitation in place:

const x: string = getSomeString();
const y = { x };
t.is(x, 123); // TS complains, this does not compile
t.not(x, 123); // Same here
t.deepEqual(y, { x: 123 }); // Same here
t.notDeepEqual(y, { x: 123 }); // Same here

So the fact is that the current types already complain when a type mismatch happens... Except in this situation of a literal union of strings being compared against another string. This is an inconsistency. So I think it is reasonable to either fix this inconsistency in the way I proposed, or maybe reconsider the type checking in the other way (making it completely loose).

I don't like the second option though. It doesn't feel right to let someone write a t.is(someString, someNumber) in TypeScript when it is statically conclusive that it will always fail. Maybe another sensible way to fix the inconsistency is to let the t.not and t.notDeepEqual receive any parameters whatsoever, while enforcing the same type only for t.is and t.deepEqual?

[papb]

Ah, and I almost forgot, but there is also the original motivation that brought me to this, which is autocomplete. I want to test a method that returns a literal union, as follows:

function getType(path: string): 'file' | 'directory' | 'symlink' | 'nothing' {
  // ...
}

t.is(getType(somePath), ''); // I want autocomplete here (inside the empty string), but I don't get it, this is very annoying!!

While if my method returned false instead of 'nothing', for example, I would get it, because the way TS infers types is different if all parts of the literal union share a same common type or not:

function getType(path: string): 'file' | 'directory' | 'symlink' | false {
  // ...
}

t.is(getType(somePath), ''); // Now I have autocomplete here...

This was so frustrating that I even thought of changing the return type of my function 😅

But for now I simply manually modified the AVA typings inside my node_modules, in exactly the same way I did in this PR.

[novemberborn]

t.is(getType(somePath), ''); // I want autocomplete here (inside the empty string), but I don't get it, this is very annoying!!

While if my method returned false instead of 'nothing', for example, I would get it, because the way TS infers types is different if all parts of the literal union share a same common type or not:

Yes that's a good use case.

I am still concerned about the breaking changes, but I will pick this up as part of #2435.

[papb]

This is an inconsistency. So I think it is reasonable to either fix this inconsistency in the way I proposed, or maybe reconsider the type checking in the other way (making it completely loose).

@novemberborn Which option here you prefer?

[novemberborn]

Which option here you prefer?

I haven't given it much thought yet, sorry. I'm blocked (due to time constraints) with #2435 anyhow.