remeda / remeda

A utility library for JavaScript and TypeScript.
http://remedajs.com
MIT License
4.26k stars 130 forks source link

`isDeepEqual` has argument-order-dependent signature overload coverage #753

Closed stwlam closed 51 minutes ago

stwlam commented 1 week ago

Playground link

declare const a1: object | null;
declare const b1: object;
isDeepEqual(a1, b1); // ok

declare const a2: object;
declare const b2: object | null;
isDeepEqual(a2, b2); // not ok: No overload matches this call
cjquines commented 1 week ago

hm… i'm not sure what the behavior should be here. the idea is that we want the second argument to extend the first argument to prevent "accidental" comparisons, but i think this is a case where we should allow this.

stwlam commented 1 week ago

Given the commutativeness of equality, maybe that ought to be given up?

cjquines commented 1 week ago

i think we would sooner give up commutativity of equality than practical usefulness. in either case, i don't think it's useful for us to disallow b if its type is the same as a but nullable.

stwlam commented 1 week ago

I guess the practical usefulness isn't clear to me. E.g., it seems perfectly sensible to expect the following to work.

declare const a3: { foo: boolean }
declare const b3: object;
isDeepEqual(a3, b3); // No overload matches this call
eranhirsch commented 51 minutes ago

Our functions are designed with pipes in mind and to surface issues as compile-time (typing) issues instead of runtime bugs. One such concern is that we want to detect when assumptions about the shape and make of an object change during the lifetime of a project, but to do this, we need to sacrifice flexibility and some ergonomics of how our types work; mainly, this manifests as "forward" typing where the first param (usually the "data" param) defines further refinements on the other params in the function signature.

The best example of this (and a function we get feedback on often) is prop:

declare const MY_ARRAY: Array<{ a: number }>;
MY_ARRAY.map(R.prop("a"))

The way prop is typed, you'd get a nice typeahead from your IDE while you are typing the prop name, but more importantly, renaming the prop would cause a typescript error:

declare const MY_ARRAY: Array<{ b: number }>;
// @ts-expect-error [ts2345] ...
MY_ARRAY.map(R.prop("a"));

https://www.typescriptlang.org/play/?#code/JYWwDg9gTgLgBAbzmKExwL5wGapHAIigFMRiATAQwIG4AoO84gYwBtKS5mIA7AZ3gBZAJoB9AIIAlSeOEAuOOKhRKATwA8SSgp4BXEACNiUTAD56dEROmyAdCEpgAFCjROC1AJSeLTNh2IuXgE4KykZMQBVAAUAEXEAFQBRWIUlFQ0kAx19IxMMcwYwmyi4xJT7RxdUZw8CbxogA

Similarly, if we want isDeepEqual to surface a compile-time error when your types change, we need the second param to rely on the first:

// @ts-expect-error [ts2345] ...
MY_ARRAY.filter(isDeepEqual({ a: 123 }));

https://www.typescriptlang.org/play/?#code/JYWwDg9gTgLgBAbzsAzgEQKYbAUQI4CuAhgDZwC+cAZlBCHAERQYgYAmRDA3AFA9sYAxiSLM4giADsU8ALIBNAPoBBAEqrl8gFxxlUKEQCeAHiREdkgiABGGKBQB8vHgpXrNAOirASMOwApUTGx8YhJ-Mx0ARgAmAGYKAEpE5wFhUQxxKRk4VzUNJQBVAAU0ZQAVHDQdPQMTJGsLK1t7cic+PPci0oqqrx8-KED0LFxCUgi4czhYhPJk3iA

We believe this is the right approach to designing the API as it brings the most value to our users.