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

fix: run task command on files deep copy #1217

Merged
merged 1 commit into from Oct 15, 2023

Conversation

louneskmt
Copy link
Contributor

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: []
}

@okonet
Copy link
Collaborator

okonet commented Sep 15, 2022

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
Copy link
Collaborator

okonet commented Sep 15, 2022

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

@louneskmt
Copy link
Contributor Author

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
Copy link
Contributor Author

louneskmt commented Sep 15, 2022

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.

@iiroj
Copy link
Member

iiroj commented Oct 14, 2023

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

lib/makeCmdTasks.js Outdated Show resolved Hide resolved
iiroj
iiroj previously approved these changes Oct 15, 2023
@iiroj iiroj merged commit 67e3854 into lint-staged:master Oct 15, 2023
14 checks passed
@github-actions github-actions bot mentioned this pull request Oct 15, 2023
@louneskmt
Copy link
Contributor Author

Thanks for the unit test and the merge!

@okonet
Copy link
Collaborator

okonet commented Oct 15, 2023

Thanks @louneskmt for your contribution!

@louneskmt louneskmt deleted the cmd-files-deep-copy branch October 15, 2023 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants