avajs / ava

Node.js test runner that lets you develop with confidence 🚀
MIT License
20.74k stars 1.41k forks source link

change deepEqual (actual: ValueType, expected: ValueType) to (any, any) #2580

Closed mesqueeb closed 3 years ago

mesqueeb commented 4 years ago

https://github.com/avajs/ava/blob/f328a6933af7aca221b08f694bb14b03701eca68/index.d.ts#L125

Because I run my Ava tests in ts-node, Ava has type checking on assertion functions. Mostly it's OK, but the type checking done by deepEqual always gets in the way.

There are so many cases where the the two objects I'm comparing are practically the same data-wise, but Type-wise the type of one is not the same as the other.

Just one example:

image

Or another:

image

You can see the types being passed are clearly the same, but somehow DeepEqualAssertion is throwing errors:

image

I believe TypeScript works best if it doesn't get in the way, and one way to solve a lot of issues I always face with deepEqual is to just simply change the function's signature to:

// instead of:
<ValueType = any>(actual: ValueType, expected: ValueType, message?: string): void; 

// do this:
(actual: any, expected: any, message?: string): void; 

I don't see any harm in this and I believe that we don't need the currently strict type checks there are in place on deepEqual. Why don't we let deepEqual be only about checking the data, not the types. That's my humble request. 🙃

sindresorhus commented 4 years ago

Makes sense to me. However, I think it should be object to ensure people don't accidentally don't try to compare primitives.

tymfear commented 4 years ago

For me this makes code analysis less reliable. It makes sense that you should compare objects with the same types, otherwise they are not equal anyway.

novemberborn commented 4 years ago

This might be similar to what @papb was bringing up in #2575. I'm reluctant though, the expected value should be structurally the same as the actual value, and it's nice to get auto-completion and type checking for it.

What I tend to do when I get stuck with the type inference in tests is cast to any. I think that strikes the right balance.

papb commented 4 years ago

Hi @novemberborn, I totally agree with you. I wouldn't like to lose autocompletion and type safety on this. When I want to assert that two things are deep equal, if TypeScript can know ahead of time that they don't even have the same time, I prefer it to fail at compile time than at runtime.

papb commented 4 years ago

Hi @mesqueeb, can you post a minimal example that represents your situation in which the types "are the same" but TypeScript thinks they are not? I couldn't understand only from your screenshots. To me, if two objects don't have the same type, then clearly t.deepEqual will fail, so I don't see this as "TypeScript getting in the way". Instead, TypeScript is helping you letting you know ahead of time that the test will fail.

novemberborn commented 3 years ago

For #2456 I'm changing this to:

<Actual, Expected extends Actual>(actual: Actual, expected: Expected, message?: string): actual is Expected;