Discussing rule: fp/no-mutating-methods

[eduardoboucas]

I find myself skipping this rule whenever I need to sort an array. Take the following example:

const applyConfigToFunction = ({ config, func }) => {
  // It's safe to mutate the array because it's local to this function.
  // eslint-disable-next-line fp/no-mutating-methods
  const matches = Object.keys(config)
    .filter((expression) => minimatch(func.name, expression))
    .map((expression) => {
      const starCount = [...expression].filter((char) => char === '*').length

      return {
        expression,
        weight: expression.length - starCount - (starCount ? 1 : 0),
      }
    })
    .sort((fA, fB) => fA.weight - fB.weight)
  const functionConfig = matches.reduce((result, { expression }) => mergeConfigObjects(result, config[expression]), {})

  return {
    [func.name]: functionConfig,
  }
}

In my view, this example adheres to the functional programming paradigm. The usage of sort does not cause any side effects. More importantly, there are no obvious alternatives to it.

What do you think?

[ehmicky]

Array.sort() mutates the array, but in the case above the array is shallowly cloned by both Array.filter() and Array.map() (also the array is created from Object.keys(), not passed by the caller).

This is an example where the mutating aspect of Array.sort() might create some unexpected behavior:

const getTitle = function(objects) {
  const [firstObject] = objects.sort((objectA, objectB) => objectA.weight - objectB.weight)
  return firstObject.title
}

Then a caller would:

console.log(objects[0])
const title = getTitle(objects)
console.log(objects[0]) // This is different now

Using objects.slice().sort() above would fix this (at a small performance cost). I think this is the type of pattern that fp/no-mutating-methods is encouraging. Unfortunately, the rule does not seem to recognize when the array has just been cloned, like in your example.

Using an utility method like here or like sort-on could help, but I agree this is verbose. :-/

[eduardoboucas]

Closing due to #206.