Correct the argument types of the forEach function

[Byeol]

According to the definition of Array.prototype.forEach function, action should return void.

[jwngr]

The type signature is correct, but the implementation of forEach() actually returns the wrong thing in certain cases. As noted in the reference documentation, forEach() should return a boolean which indicates if execution was cut short by action() returning true at some point. However, as forEach() is implemented, it always returns true. We need to update the method to check the return value of action(), and, if it is true, stop execution and return true ourselves.

I created #15 to track this and am assigning it over to @laurenzlong for now.

cc/ @inlined - Since the relevant TODO at the top of this method's implementation has your name in it.

[jwngr]

@Byeol - If you'd like to update this pull request given my explanation above, we would be happy to accept your contribution. We should probably include some tests here for different scenarios as well.

[Byeol]

@jwngr You're right. I updated the implementation of forEach() to make it correspond to the reference documentation.

According to MDN, There is no way to stop or break a forEach() loop other than by throwing an exception. So I used some() method, which returns true if the callback function returns a truthy value for any array element; otherwise, false. some() immediately returns true if the callback function returns a truthy value for any array element.

[jwngr]

Thanks for the updates! I had a few comments for some edge cases and a way to simplify the code with a Lodash helper.

[jwngr]

Lodash has a helper _.some() which we can use directly, instead of having to do _.keys(val).some().

[jwngr]

Also, I think we want to make sure this behavior only happens when true is returned, not when a truthy value (such as "a" or 1) is returned. So, this should probably end with action(this.child(key) === true. Can you please add a test for the case where action() returns 1 to ensure this behavior is correct?

[jwngr]

Can you add some checks here that the return values of both forEach() calls in this test are false?

[Byeol]

@jwngr Thanks for the review! I updated the implementation to cancel further enumeration only if the callback returns explicit true, and simplified the code with a Lodash helper _.some().

[jwngr]

Looks great to me! Thanks for iterating on this with me. I'm going to assign this over to @rjhuijsman and @laurenzlong so they can merge this in when they are ready for it.