Skip to content

fix(compat/forEach): remove redundant array check in key resolution#1869

Open
Antoliny0919 wants to merge 1 commit into
toss:mainfrom
Antoliny0919:remove-redundant-array-check
Open

fix(compat/forEach): remove redundant array check in key resolution#1869
Antoliny0919 wants to merge 1 commit into
toss:mainfrom
Antoliny0919:remove-redundant-array-check

Conversation

@Antoliny0919

Copy link
Copy Markdown
Contributor

Since arrays and array like objects are handled the same way, I think isArrayLike alone should be sufficient.

@vercel

vercel Bot commented Jul 10, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
es-toolkit Ready Ready Preview, Comment Jul 10, 2026 6:11am

Request Review

Comment on lines 151 to 153
if (!collection) {
return collection;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is only intended to handle null and undefined.

With the current implementation, it would also treat other falsy values as empty, which seems broader than intended.

Would it make sense to explicitly handle only null and undefined instead?

index f0f6fb48..8aa3bbcf 100644
--- a/src/compat/array/forEach.ts
+++ b/src/compat/array/forEach.ts
@@ -144,7 +144,7 @@ export function forEach<T>(
   collection: ArrayLike<T> | Record<any, any> | string | null | undefined,
   callback: (item: any, index: any, arr: any) => unknown = identity
 ): ArrayLike<T> | Record<any, any> | string | null | undefined {
-  if (!collection) {
+  if (collection == null) {
     return collection;
   }

One thing I'm curious about: do you have a preference between using a raw collection == null check and using isNil for this kind of logic?

Personally, I think using isNil is perfectly reasonable. In fact, I'd be happy to standardize on isNil for null/undefined checks going forward.

index f0f6fb48..a1a93f68 100644
--- a/src/compat/array/forEach.ts
+++ b/src/compat/array/forEach.ts
@@ -1,5 +1,6 @@
 import { identity } from '../../function/identity.ts';
 import { range } from '../../math/range.ts';
+import { isNil } from '../../predicate/isNil.ts';
 import { ArrayIterator } from '../_internal/ArrayIterator.ts';
 import { ListIterator } from '../_internal/ListIterator.ts';
 import { ObjectIterator } from '../_internal/ObjectIterator.ts';
@@ -144,7 +145,7 @@ export function forEach<T>(
   collection: ArrayLike<T> | Record<any, any> | string | null | undefined,
   callback: (item: any, index: any, arr: any) => unknown = identity
 ): ArrayLike<T> | Record<any, any> | string | null | undefined {
-  if (!collection) {
+  if (isNil(collection)) {
     return collection;
   }

Using isNil does introduce a dependency on that utility, but I think there are both pros and cons to that. Since isNil is such a fundamental utility that's unlikely to change, I also feel it makes the intent more explicit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant