React-sortable-hoc: ArrayMove mutates array

[max-mykhailenko]

We should avoid object mutations
https://github.com/clauderic/react-sortable-hoc/blob/master/src/utils.js#L1-L10

[clauderic]

Completely agree. However, the other solutions I looked at were incredibly expensive. Happy to review any PR you may have that would solve this 👍

Thankfully arrayMove is an opt-in helper, so you're free to use your own implementation as well

[robbinjanssen]

Why not just 'copy' the array? const new = [].concat(array)

[zaygraveyard]

Here are three possible implementations of an immutable arrayMove.
On Chrome v53 arrayMoveImmutable() is the fastest.
But on Firefox v49 arrayMoveCopy() is the fastest (and the arrayMoveImmutableReverse() is faster then arrayMoveImmutable()).

function arrayMoveCopy(array, previousIndex, newIndex) {
  return arrayMove(array.slice(), previousIndex, newIndex);
}

function arrayMoveImmutable(array, previousIndex, newIndex) {
  var length = array.length;
  var newLength = newIndex >= length ? newIndex + 1 : length;
  var newArray = new Array(newLength);
  var element = array[previousIndex];

  for (var i = 0, j = 0; i < newLength; i++) {
    if (i === newIndex) {
      newArray[i] = element;
    } else {
      if (j === previousIndex) {
        j++;
      }

      if (j < length) {
        newArray[i] = array[j++];
      } else {
        newArray[i] = undefined;
      }
    }
  }

  return newArray;
}

function arrayMoveImmutableReverse(array, previousIndex, newIndex) {
  var length = array.length;
  var newLength = newIndex >= length ? newIndex + 1 : length;
  var newArray = new Array(newLength);
  var element = array[previousIndex];
  var i = newLength;
  var j = newLength;

  while (i--) {
    if (i === newIndex) {
      newArray[i] = element;
    } else {
      j--;
      if (j === previousIndex) {
        j--;
      }

      if (j < length) {
        newArray[i] = array[j];
      } else {
        newArray[i] = undefined;
      }
    }
  }

  return newArray;
}

[max-mykhailenko]

@zaygraveyard my solution was based on slicing and concating array parts, but your solution with one level loop should have better performance

[yacodes]

I've created a simple solution for this issue here, but i guess we better measure performance for every example above, so we can understand which implementation is the fastest.

[clauderic]

Thanks for your PR @canvaskisa, it's been merged back into master.

Regarding performance concerns: I think if performance is a real concern for your application (for instance, if you have an array with millions of records), you're free to opt out and use your own implementation, as arrayMove is just a convenient, opt-in helper.

Thanks for your input everyone