remeda / remeda

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

`isEmpty` type definition don't work correctly with interfaces #879

Open AndreaPontrandolfo opened 1 week ago

AndreaPontrandolfo commented 1 week ago

I'm really struggling with isEmpty.

I think the current implementation is seriously lacking from a Typescript perspective. Either that, or i'm not understanding it clearly (then might the docs be the problem).

The docs state this very basic example:

R.isEmpty({}); //=> true

Which is fine. and works as expected, but let's just assign any kind of interface to it:

interface MyObject {
  stuff1?: string;
  stuff2?: number;
}

const x: MyObject = {};

R.isEmpty(x);

Suddenly i'm met with a Typescript error:

No overload matches this call.
  Overload 1 of 3, '(data: string | undefined): data is "" | undefined', gave the following error.
    Argument of type 'MyObject' is not assignable to parameter of type 'string'.
  Overload 2 of 3, '(data: IterableContainer): data is []', gave the following error.
    Argument of type 'MyObject' is not assignable to parameter of type 'IterableContainer'.
  Overload 3 of 3, '(data: Readonly<Record<PropertyKey, unknown>>): data is Record<PropertyKey, never>', gave the following error.
    Argument of type 'MyObject' is not assignable to parameter of type 'Readonly<Record<PropertyKey, unknown>>'.
      Index signature for type 'string' is missing in type 'MyObject'.ts(2769)

I have no idea what any of that TS gibberish means, but clearly this isn't expected behavior, right? I'm still just passing a simple empty object, why is this failing at all?

eranhirsch commented 1 week ago

I agree that the typing for isEmpty isn't very good, as it doesn't do enough introspection into the original type and instead attempts to reconstruct the input in a way that makes it impossible not to be empty. This leaves a lot of things to be desired; for example, checking emptiness on an object that can't be empty (like { a: number }) should result in never and not { a: never }.

The function also isn't tested very thoroughly in our type tests.

Regardless, We often don't look into (or test) objects typed via interface because they introduce some complexities that type definitions don't. This is primarily due to https://www.typescriptlang.org/docs/handbook/declaration-merging.html. Typescript takes a safer approach and infers wider types when types derived from interface are inferred but provides tighter typing when type is used. I recommend turning on https://typescript-eslint.io/rules/consistent-type-definitions and setting it to "type" instead of the default "interface" and only use interfaces when you know you need declaration merging.

AndreaPontrandolfo commented 1 week ago

I DO have consistent-stype-definitions on in all my projects, but I go the opposite direction and I leave it set to it's default, which is "interface".

We could stay here literally all day debating which is the best between interface or type, but... i don't think this is even remotely the point...? Remeda shouldn't be concerned at all whether i'm using interface or type in my codebase, it should support both, equally. And i would consider this a basic point for the library, not even debatable, honestly.

To be clear, at no point in the docs Remeda states that interfaces aren't supported. Remeda states:

First-class TypeScript support

But then interfaces are not supported? I don't think this is right, and if it was, it should at the very least be stated in the docs.

If supporting interfaces isn't in the objectives of the library, i would drop the library immediately, because stopping using interfaces all together just because Remeda doesn't support them is unreasonable and unfeasible for me. This would definitely be a deal breaker for me.

cjquines commented 1 week ago

my apologies @AndreaPontrandolfo. i'm not sure what about eran's response gave you the idea that supporting interfaces isn't one of our objectives, but do understand that it's tricky to support in several cases. of course, it does sound like you have much more experience working with interfaces than either of us have, so if you have an ideas about how to support interfaces here then a pr fixing this would be much appreciated

eranhirsch commented 1 week ago

@AndreaPontrandolfo, I was referring specifically to concrete issues with how types differ from interfaces, not a judgment of preferences.

Take a look at this playground example:

https://www.typescriptlang.org/play/?#code/JYOwLgpgTgZghgYwgAgJLIN4ChnLgLmQGcwpQBzAbiwF8sswBPABxQBVkBeTHPQksiCq16TVsgAaXZACUICAPZQAJgB4AClAWsoTANIRGAGmQBXEAGsQCgO4gAfNQYsUqCB27oIAD0ghlRMgcAPzIpKYohPAANkQQ1AD0CbjIAHrBzuJsEOjcHD5+AWjIoeGRyDFxicm46aIuaBBSnsgFEP6BUqVQEchRcLHxWEkpdWIoEjnSUm0dxd29-YPVoxmZ7E3S+b7tRV1hPeWVQyO1GeOS7tOtO3MhB4sVA1XDNWnBQA

You can see that although the types extend each other (which means they are "equal"), this equality is not transitive, and the types have material differences as far as typescript is concerned. This is just a quirk of how the language works.

Remeda is supported by a tiny team of just two part-time maintainers and relies heavily on community contribution. We are always happy to support more cases but are limited by our capacity. The complexity that interfaces add means that when we prioritize our work, we sometimes have to skip these edge cases. This is not ideal, and we are always happy to accept any PRs that address this.

eranhirsch commented 1 week ago

https://www.totaltypescript.com/type-vs-interface-which-should-you-use

AndreaPontrandolfo commented 1 week ago

https://www.totaltypescript.com/type-vs-interface-which-should-you-use

From https://www.totaltypescript.com/type-vs-interface-which-should-you-use#default-to-type-not-interface:

But the TS team recommends you default to using interface and only use type when you need to.

Again, i'm not gonna argue about the use of interfaces and types, both should be supported out of the box, this is a basic expectation for this library and it shouldn't even be debatable.