Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

isEmpty doesn't accept falsy input #369

Closed
TkDodo opened this issue Aug 28, 2023 · 9 comments · Fixed by #377
Closed

isEmpty doesn't accept falsy input #369

TkDodo opened this issue Aug 28, 2023 · 9 comments · Fixed by #377
Labels

Comments

@TkDodo
Copy link
Collaborator

TkDodo commented Aug 28, 2023

we ran into this issue today, consider the following:

type User = {
  id: number
  firstName: string | undefined
}
declare function getUser(id: number): User | undefined

const user = getUser(1)
const hasFirstName = isEmpty(user?.firstName)

this errors because we can't pass undefined to isEmpty (TypeScript playground). One workaround is to either check upfront explicitly:

const hasFirstName = user?.firstName ? isEmpty(user.firstName) : false

or coerce to empty string:

const hasFirstName = isEmpty(user?.firstName ?? '')

both of which are kinda weird. Imo, "a function that checks if the passed parameter is empty" (from the docs) could consider accepting null and undefined and return true for them being "empty".

What do you think @eranhirsch ?

@TkDodo
Copy link
Collaborator Author

TkDodo commented Aug 28, 2023

here's a diff that I think would accomplish this:

diff --git a/src/isEmpty.ts b/src/isEmpty.ts
index b42b70c..fc73d57 100644
--- a/src/isEmpty.ts
+++ b/src/isEmpty.ts
@@ -22,6 +22,7 @@ export function isEmpty(data: ReadonlyArray<unknown> | []): data is [];
@@ -22,6 +22,7 @@ export function isEmpty(data: ReadonlyArray<unknown> | []): data is [];
 export function isEmpty<T extends Readonly<Record<PropertyKey, unknown>>>(
   data: T
 ): data is Record<keyof T, never>;
+export function isEmpty(data: null | undefined): true;
 export function isEmpty(data: unknown): boolean {
   if (isArray(data) || isString(data)) {
     return data.length === 0;
@@ -29,5 +30,5 @@ export function isEmpty(data: unknown): boolean {
   if (isObject(data)) {
     return Object.keys(data).length === 0;
   }
-  return false;
+  return data == null;
 }

@eranhirsch
Copy link
Collaborator

When I reviewed the original PR my fear was that any relaxation of typing would allow some bugs to fall through, this is the reasoning behind typescripts strictest configs which are not enabled by default (like exactOptionalPropertyTypes). Zod have the same issue where optional() object properties return a type which has undefined in it.

Because remeda isn't configurable, we need to make a decision on how strict our types should be. I prefer strictness, but understand this isn't everyone's cup of tea.

Your example could be shortened to:

const hasFirstName = user === undefined ? false : isEmpty(user.firstName);

and I think that's descriptive enough, think about a different use-case. It's not always obvious what the undefined case should return, it's not always going to be false.

const hideWelcomeGreeting = user === undefined ? true : isEmpty(user.firstName);

@TkDodo
Copy link
Collaborator Author

TkDodo commented Aug 28, 2023

Your example could be shortened to:

const hasFirstName = user === undefined ? false : isEmpty(user.firstName);

not exactly because the firstName itself is also optional.

Because remeda isn't configurable, we need to make a decision on how strict our types should be.

we always have the option of adding stricter types to the .strict namespace?

@eranhirsch
Copy link
Collaborator

true, but that doesn't address what should be the default returned value for isEmpty(undefined), is it true or false?

@TkDodo
Copy link
Collaborator Author

TkDodo commented Aug 28, 2023

I think it should be true, given that we agree that undefined and null can both be considered empty (for whatever empty means in that terminology 😅 )

@eranhirsch
Copy link
Collaborator

Facebook's Hack standard library define null as true for strings (and strings only) and doesn't accept nulls for collections. I don't mind going with this...

https://docs.hhvm.com/hsl/reference/function/HH.Lib.C.is_empty/
https://docs.hhvm.com/hsl/reference/function/HH.Lib.Str.is_empty/

@TkDodo
Copy link
Collaborator Author

TkDodo commented Aug 28, 2023

yeah I would also not allow null for collection-only methods like flatMap (I think we had a separate discussion about this), but here, were strings are also allowed, I think it would be a better DX.

@eranhirsch
Copy link
Collaborator

Checkout the #377 that solves this. My only issue is that we will now accept isEmpty(undefined) too, and I couldn't find a way around it so users lose some protection against cases like: isEmpty(obj["typo"])

@TkDodo
Copy link
Collaborator Author

TkDodo commented Sep 1, 2023

🎉 This issue has been resolved in version 1.26.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@TkDodo TkDodo added the released label Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants