`isEmpty`: disallow strings

[AndreaPontrandolfo]

I strongly believe isEmpty shouldn't accept a string as input.

isEmpty is supposed to be used only on objects (and arrays by extension). Reason being, that it indicates that a collection or dictionary has no items inside. Doing this check manually is verbose and somewhat non-trivial, which is why this method is so useful. But strings are a primitive and testing for their emptiness is in-fact as trivial as: myString === ""

One could say: "Oh, but we are actually using it as a convenience method to check if the value is falsy AND ALSO eventually empty". But even in this case this would be false/inconsistent because in the current implementation isEmpty doesn't accept numbers (meaning that we don't account to check for 0 values). This behavior already sparked confusion multiple times within my team. The more the constraints of a method are clearly defined and easily visible, the better it is.

I can provide a PR if this is accepted.

[eranhirsch]

Why does the fact that it works for strings matter in cases where you use it on objects or arrays? Do you have types that are both strings and arrays? string | string[]? The advantage of having isEmpty for strings is that it could be used in dataLast invocations without requiring naming temporary variables:

filter(isEmpty)

// vs
filter((x): x is "" => x === "")

Can you please describe what problem your team is encountering?

[AndreaPontrandolfo]

Can you please describe what problem your team is encountering?

It's just a readability and predictability issue. When i skim quickly through the code and see isEmpty i immediately assume that we are dealing with objects and arrays (collections of items ) in that part of the code, while with the current implementation i cannot be sure, because it could also be strings, meaning that i have passively less understanding of the context of that part of the codebase and i need to actively look at the types to see what we are dealing with. It's like conventions like uppercasing constants or prefixing booleans with is, you don't have to do that with TS, because the type already tell you that, but it makes the code much more readable at a glance. It's all about expectations.

The advantage of having isEmpty for strings is that it could be used in dataLast invocations without requiring naming temporary variables

I see, you are right, i totally didn't think about this.

I think i can add a rule option to the eslint-plugin-remeda/prefer-is-empty rule, where i disallow strings as input of isEmpty unless we are inside a pipe.

Thank you for your explanation, i think we can close this.

[eranhirsch]

Taking inspiration from the JS library (which is far from being a well-thought-out API, but it's the world we live in), you can see that String.prototype and Array.prototype have many functions and prototypes in common: length, at, includes, indexOf, slice, concat, all of which would have the same problem you raise in this issue where when reading the code, you can't tell what the type is by the function name. Even in native JS you wouldn't be able to tell if myThingy.length === 0 is a string or an array.

When designing the API for Remeda we try to stay as close to native JS as possible. We only split functions when it's hard to type both cases at the same time. Instead of having isEmptyString, isEmptyArray, isEmptyObject we opted for isEmpty.

This is also the case for Ramda's isEmpty which this function was modeled after.