lint-staged icon indicating copy to clipboard operation
lint-staged copied to clipboard

fix: run task command on files deep copy

Open louneskmt opened this issue 2 years ago • 4 comments

This prevents the task command to change the files array by inadvertence. It happened to me and I took a long moment to diagnose the issue.

Task in generateTasks
{
  pattern: 'apps/dummy/dummy/**/*.js',
  commands: [Function: apps/dummy/dummy/**/*.js],
  fileList: [
    '/Users/lounesksouri/dummy/apps/dummy/dummy/src/config.js'
  ]
}
Task in runAll after makeCmdTasks
{
  pattern: 'apps/dummy/dummy/**/*.js',
  commands: [Function: apps/dummy/dummy/**/*.js],
  fileList: []
}

louneskmt avatar Sep 14 '22 10:09 louneskmt

So you're saying something can mutate the list of files? Do you have a reproduction case? I'm wondering how this is possible. Also let's create an issue for this first and reference the PR.

okonet avatar Sep 15 '22 08:09 okonet

Also before we can merge it we would need a test for this.

okonet avatar Sep 15 '22 08:09 okonet

So you're saying something can mutate the list of files? Do you have a reproduction case? I'm wondering how this is possible.

Yeah, I faced this issue running Array.prototype.splice on a reference to filenames in a subsequent function. This method directly modifies the object.

Here is an example:

const divideArray = (arr, size) => {
  return new Array(Math.ceil(arr.length / size))
    .fill()
    .map((_) => arr.splice(0, size))
}

const splitCommand = (filenames, commandFormat) => {
  const splitFilenames = divideArray(filenames, 5)
  return splitFilenames.map((files) => `${commandFormat} ${files.join(' ')}`)
}

module.exports = {
  '*.js': (filenames) => [
    "pnpm prettier --write '*.js'",
    ...splitCommand(filenames, 'pnpm run --silent eslint --fix'),
  ],
}

This basically outputs the same command applied to a subset of the filenames. If there are 10 changed files, the splitCommand will output an array of commands applied to files 5 by 5.

You can see that the files has changed by logging the variabe right before and after the following line: https://github.com/okonet/lint-staged/blob/17c51aff00ea73f9588132c28eedbce535ee1ad8/lib/makeCmdTasks.js#L46

Not sure to have the time to add a test this week.

louneskmt avatar Sep 15 '22 20:09 louneskmt

Deep copying the array indeed solves the issue:

const divideArray = (arr, size) => {
  const arrCopy = [...arr]
  return new Array(Math.ceil(arrCopy.length / size))
    .fill()
    .map((_) => arrCopy.splice(0, size))
}

But it would be definitely better if the library handled that case internally.

louneskmt avatar Sep 15 '22 20:09 louneskmt

Ping @louneskmt I rebased this PR and added a changeset file!

iiroj avatar Oct 14 '23 20:10 iiroj

Thanks for the unit test and the merge!

louneskmt avatar Oct 15 '23 07:10 louneskmt

Thanks @louneskmt for your contribution!

okonet avatar Oct 15 '23 07:10 okonet