js-polyfills icon indicating copy to clipboard operation
js-polyfills copied to clipboard

Callback bug in findLastIndex

Open robinspin opened this issue 9 months ago • 0 comments

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!

robinspin avatar Feb 20 '25 18:02 robinspin