Logic around array removeKey seems wrong

[0x80]

It seems that the logic around removing items from an array is seriously dodgy.

For example, in the test you go from toys ["car", "doll", "car"] to [] by doing 3 remove actions on the array. But the actions are not looking at the items in the array, they will try to remove an item from the index where the original item was positioned, and if that index doesn't have an element, it will just remove the last element from the array, whatever it is.

Here's what happens if you log the applyLeafChange arguments. The first is the array contents it operates on, and the second is the change to be applied.

1. applyLeafChange [ 'car', 'doll', 'car' ] { type: 'remove', key: '0', value: 'car' }
2. applyLeafChange [ 'doll', 'car' ] { type: 'remove', key: '1', value: 'doll' }
3. applyLeafChange [ 'doll' ] { type: 'remove', key: '2', value: 'car' }

As you can see the first mutation is valid, because there is a "car" at index 0, but 2 and 3 are not. Step two should remove the doll, but instead removes the car at the end. Then step 3 should remove the car at index 2 but there is no car and not index 2 anymore, and instead it just removes the last item.

The end result is still [], and that's why the test passes, but this all seems wrong to me.

I think what you intended to do with the indexOfItemInArray function is to look for the actual value in the array. But for that to work you need to pass the value to removeKey which is currently not happening.

This is what I ended up with:

function removeKey(obj: JsonObject | JsonArray, key: string, value: JsonValue) {
  if (Array.isArray(obj)) {
    if (!obj[Number(key)]) {
      const index = obj.findIndex((x) => x === value);
      return obj.splice(index, 1);
    } else {
      return obj.splice(Number(key), 1);
    }
  } else {
    return delete obj[key];
  }
}

In this implementation, if the item can not be found, it will still remove the last item from the array whatever it is, so it's probably better to catch the -1 returned from findIndex and throw an error.

This code is the result of converting your library to Typescript and in the process, I have also stripped the logic around embeddedKeys because I didn't need that complexity and it was difficult enough trying to port this.

In the future, I would probably move to a JSON Patch compatible implementation, but my production code was already dependent on this library so I decided to convert it to something I can understand. If enough people are interested I might release it in the future.

[0x80]

Here is an implementation that I think might be correct. It does a deep comparison on the value and throws and error if the item can not be found in the array.

function removeKey(obj: JsonObject | JsonArray, key: string, value: JsonValue) {
  if (Array.isArray(obj)) {
    if (!isEqual(obj[Number(key)], value)) {
      const index = obj.findIndex((x) => isEqual(x, value));
      assertMsg(
        index !== -1,
        `Failed to find value ${value} to remove from array ${JSON.stringify(
          obj,
        )}`,
      );
      return obj.splice(index, 1);
    } else {
      return obj.splice(Number(key), 1);
    }
  } else {
    return delete obj[key];
  }
}