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

Inconsistent index after .remove() / .delete() #18

Open
alexandernst opened this issue Sep 12, 2024 · 5 comments
Open

Inconsistent index after .remove() / .delete() #18

alexandernst opened this issue Sep 12, 2024 · 5 comments

Comments

@alexandernst
Copy link

Using .remove() or .delete() on arrays leads to inconsistent index of the following elements in the array.

POC:

const obj = [
  () => {},
  () => {},
  "a",
];

traverse(obj).forEach(function (x) {
  if(typeof x === "function") this.remove();
});

console.dir(obj);

The expected output is [ 'a' ], but the actual output is [ Function (anonymous), 'a' ]

@ljharb
Copy link
Owner

ljharb commented Sep 12, 2024

It doesn't seem like this ever worked with any version of traverse, and in general, JS traversal semantics become unreliable when you mutate something mid-iteration.

However, even with immutable: true, it fails - in the mutable case, it only removes the first function; in the immutable case, it only removes the second function. Both behaviors are bizarre and surprising.

@alexandernst
Copy link
Author

It doesn't seem like this ever worked with any version

Yes, the bug appears to have been dragged from the very start.

JS traversal semantics become unreliable when you mutate something mid-iteration.

I do agree that mutating inside of a traversal operation could be unreliable, but in this particular code/case it can be reliable. Arrays are always walked from left to right (from index 0 to length - 1) and elements inside the array can be removed only with .remove(). When the algorithm is iterating an array, it assigns a key to each element in the state object. In my POC that would be:

"0"  -  () => {}
"1"  -  () => {}
"2"  -  "a"

Let's say that .remove() is called on the first element. The keys of the next elements should be decreased by 1:

.
"0"  -  () => {}
"1"  -  "a"

Of course, the iterator should also be rewinded by 1 to prevent it from skipping 1 element.

@ljharb
Copy link
Owner

ljharb commented Sep 12, 2024

I agree with you, this is a bug worth fixing.

@ljharb ljharb closed this as completed in ca14874 Sep 12, 2024
@alexandernst
Copy link
Author

Great!! I'll make sure to test the new version and report back when you release it! 🙏

@alexandernst
Copy link
Author

I just tested this and the fix doesn't seem to be handling it.

const obj = [
  () => {},
  () => {},
  "a",
  () => {},
];

traverse(obj).forEach(function (x) {
  if(typeof x === "function") this.remove();
});

console.dir(obj);

The output is [ 'a', [Function (anonymous)] ]

@ljharb ljharb reopened this Sep 13, 2024
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

2 participants