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

Callback bug in findLastIndex #9

Open
robinspin opened this issue Feb 20, 2025 · 0 comments
Open

Callback bug in findLastIndex #9

robinspin opened this issue Feb 20, 2025 · 0 comments

Comments

@robinspin
Copy link

Hi Siddhi,

The findLastIndex function has a small bug where it invokes callback with undefined as the first argument before moving on to the other items. Not always a problem, but if the callback is expecting an object then it can cause exceptions to be thrown - e.g. for callback element => element.foo = 'bar'.

The fix is changing this:

  for (let i = this.length; i >= 0; i--) {

to this:

  for (let i = this.length - 1; i >= 0; i--) {

We can also add the following test to make sure this is fixed and won't happen again:

  test("invokes callback in the expected way", () => {
    const arr = [123, 456, 789];
    const callback = jest.fn(() => false);
    arr.myFindLastIndex(callback);
    expect(callback).toHaveBeenCalledTimes(3);
    expect(callback).toHaveBeenNthCalledWith(1, 789, 2, arr);
    expect(callback).toHaveBeenNthCalledWith(2, 456, 1, arr);
    expect(callback).toHaveBeenNthCalledWith(3, 123, 0, arr);
  });

And also maybe this to make sure we're not calling callback more often than needed:

  test("does not invoke callback after element found", () => {
    const arr = [123, 456, 789];
    const callback = jest.fn((element) => element === 456);
    arr.myFindLastIndex(callback);
    expect(callback).toHaveBeenCalledTimes(2);
    expect(callback).toHaveBeenNthCalledWith(1, 789, 2, arr);
    expect(callback).toHaveBeenNthCalledWith(2, 456, 1, arr);
  });

I was going to open a PR for you, but I don't have permission, hopefully the above is simple for you to use anyway.

Thanks!

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

No branches or pull requests

1 participant