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

Files altered by linting tools are not commited anymore in v9.0.1 #647

Closed
Kocal opened this issue Jul 2, 2019 · 9 comments

Comments

Projects
None yet
3 participants
@Kocal
Copy link

commented Jul 2, 2019

Hi,

Description

After upgrading to lint-staged 9 and updated our config file from:

{
  "relative": true,
  "linters": {
    "*.php": [
      "./vagrant-wrapper.sh php bin/php-cs-fixer fix --config .php_cs.dist --diff",
      "./vagrant-wrapper.sh php bin/phpstan analyse --level 7 --no-ansi --no-progress",
      "git add"
    ],
    "assets/**/*.{js,jsx,vue}": [
      "eslint --fix",
      "git add"
    ],
    "*.{json,yml,yaml,md}": [
      "prettier --write",
      "git add"
    ]
  }
}

to this (the old way was better, but w/e 😞) :

const path = require('path');

function makeRelativePaths(absolutePaths) {
  const cwd = process.cwd();

  return absolutePaths.map(file => path.relative(cwd, file));
}

module.exports = {
  '*.php': (absolutePaths) => {
    const relativePaths = makeRelativePaths(absolutePaths);

    return [
      `./vagrant-wrapper.sh php bin/php-cs-fixer fix --config .php_cs.dist --diff ${relativePaths.join(' ')}`,
      `./vagrant-wrapper.sh php bin/phpstan analyse --level 7 --no-ansi --no-progress ${relativePaths.join(' ')}`,
      `git add ${relativePaths.join(' ')}`,
    ];
  },
  'assets/**/*.{js,jsx,vue}': (absolutePaths) => {
    const relativePaths = makeRelativePaths(absolutePaths);

    return [
      `eslint --fix ${relativePaths.join(' ')}`,
      `git add ${relativePaths.join(' ')}`,
    ];
  },
  '*.{json,yml,yaml,md}': (absolutePaths) => {
    const relativePaths = makeRelativePaths(absolutePaths);

    return [
      `prettier --write ${relativePaths.join(' ')}`,
      `git add ${relativePaths.join(' ')}`,
    ];
  },
};

It seems that files are not added anymore to the commit after being processed by ESLint, Prettier, PHP-CS-Fixer, [...].

Peek 02-07-2019 13-58

Running git diff, the file has been linted but not commited:
Sélection_925

Steps to reproduce

  • Edit a file that will be matched by linters glob patterns
  • Git add this file
  • Git commit this file
  • Husky triggers lint-staged, that will execute some commands (ESLint + git add in my case)
  • The file is commited, but not the linted version (after being processed by ESLint)

Debug Logs

expand to view
husky > pre-commit (node v11.5.0)
  lint-staged:bin Running `lint-staged@9.0.1` +0ms
  lint-staged Loading config using `cosmiconfig` +0ms
  lint-staged Successfully loaded config from `/path/to/app/.lintstagedrc.js`:
  lint-staged { '*.php': [Function: *.php],
  lint-staged   'assets/**/*.{js,jsx,vue}': [Function: assets/**/*.{js,jsx,vue}],
  lint-staged   '*.{json,yml,yaml,md}': [Function: *.{json,yml,yaml,md}] } +6ms
  lint-staged:cfg Validating config +0ms
Running lint-staged with the following config:
{
  '*.php': (absolutePaths) => {
    const relativePaths = makeRelativePaths(absolutePaths);

    return [
      `./vagrant-wrapper.sh php bin/php-cs-fixer fix --config .php_cs.dist --diff ${relativePaths.join(' ')}`,
      `./vagrant-wrapper.sh php bin/phpstan analyse --level 7 --no-ansi --no-progress ${relativePaths.join(' ')}`,
      `git add ${relativePaths.join(' ')}`,
    ];
  },
  'assets/**/*.{js,jsx,vue}': (absolutePaths) => {
    const relativePaths = makeRelativePaths(absolutePaths);

    return [
      `eslint --fix ${relativePaths.join(' ')}`,
      `git add ${relativePaths.join(' ')}`,
    ];
  },
  '*.{json,yml,yaml,md}': (absolutePaths) => {
    const relativePaths = makeRelativePaths(absolutePaths);

    return [
      `prettier --write ${relativePaths.join(' ')}`,
      `git add ${relativePaths.join(' ')}`,
    ];
  }
}
  lint-staged:run Running all linter scripts +0ms
  lint-staged:git Running git command [ 'rev-parse', '--show-toplevel' ] +0ms
  lint-staged:run Resolved git directory to be `/path/to/app` +13ms
  lint-staged:git Running git command [ 'diff', '--staged', '--diff-filter=ACM', '--name-only' ] +13ms
  lint-staged:run Loaded list of staged files in git:
  lint-staged:run [ 'assets/app/index.js' ] +8ms
  lint-staged:gen-tasks Generating linter tasks +0ms
  lint-staged:gen-tasks Generated task:
  lint-staged:gen-tasks { pattern: '*.php', commands: [Function: *.php], fileList: [] } +2ms
  lint-staged:gen-tasks Generated task:
  lint-staged:gen-tasks { pattern: 'assets/**/*.{js,jsx,vue}',
  lint-staged:gen-tasks   commands: [Function: assets/**/*.{js,jsx,vue}],
  lint-staged:gen-tasks   fileList: [ '/path/to/app/assets/app/index.js' ] } +2ms
  lint-staged:gen-tasks Generated task:
  lint-staged:gen-tasks { pattern: '*.{json,yml,yaml,md}',
  lint-staged:gen-tasks   commands: [Function: *.{json,yml,yaml,md}],
  lint-staged:gen-tasks   fileList: [] } +0ms
Stashing changes... [started]
  lint-staged:git Running git command [ 'status', '--porcelain' ] +16ms
Stashing changes... [skipped]
→ No partially staged files found...
Running linters... [started]
Running tasks for *.php [started]
Running tasks for assets/**/*.{js,jsx,vue} [started]
Running tasks for *.{json,yml,yaml,md} [started]
Running tasks for *.php [skipped]
→ No staged files match *.php
  lint-staged:make-cmd-tasks Creating listr tasks for commands [Function: assets/**/*.{js,jsx,vue}] +0ms
Running tasks for *.{json,yml,yaml,md} [skipped]
→ No staged files match *.{json,yml,yaml,md}
eslint --fix [file] [started]
  lint-staged:task cmd: eslint +0ms
  lint-staged:task args: [ '--fix', 'assets/app/index.js' ] +0ms
  lint-staged:task execaOptions: { preferLocal: true, reject: false, shell: false } +0ms
  lint-staged:task cmd: git +4ms
  lint-staged:task args: [ 'add', 'assets/app/index.js' ] +0ms
  lint-staged:task execaOptions: { preferLocal: true, reject: false, shell: false } +0ms
eslint --fix [file] [completed]
Running tasks for assets/**/*.{js,jsx,vue} [completed]
Running linters... [completed]
  lint-staged linters were executed successfully! +2s
[chore/deps-js 1deeb9de] test
 1 file changed, 3 insertions(+), 1 deletion(-)

Environment

  • OS: Debian 9.9 (Stretch)
  • Node.js: v11.5.0
  • lint-staged: 9.0.1

That's weird because in debug logs we can see that git add assets/app/index.js... 🤔

Thanks! :)

@Kocal Kocal changed the title `git add` is not working anymore since release 9 Files altered by linting tools are not commited in v9 Jul 2, 2019

@Kocal Kocal changed the title Files altered by linting tools are not commited in v9 Files altered by linting tools are not commited anymore in v9 Jul 2, 2019

@iiroj

This comment has been minimized.

Copy link
Collaborator

commented Jul 2, 2019

Thanks for the clear report! I’ll check it out. One thing that comes to mind is that possibly the commands are run parallel and not sequentially in this case.

Also, your example is a good example of the power of relative: true. Does your setup not work on absolute paths?

@Kocal

This comment has been minimized.

Copy link
Author

commented Jul 2, 2019

Ah, I didn't think about parallelism. That's probably the issue but I don't know how to fix it yet.

Yes relative: true was so much useful, I'm a bit sad that it disappears but that's the life. 😝
Nope it does not work with absolute paths:

  1. We use a VM to run our Symfony app, so we want to make sure every commands are ran inside the VM (with the vagrant-wrapper.sh, but we didn't use it everywhere because.... we forgot to 😄)
  2. I wanted to keep commiting inside or outside the VM, so we used relative: true
@iiroj

This comment has been minimized.

Copy link
Collaborator

commented Jul 2, 2019

I cannot reproduce this. On the lint-staged repo, at commit 6c1e42f, when I add the following .lintstagedrc.js config:

module.exports = {
  '*.{js,json,md}': ['prettier --write', 'git add'],
  '*.js': files => [`npm run lint:base --fix ${files.join(' ')}`, `git add ${files.join(' ')}`],
  '.*{rc, json}': ['jsonlint', 'git add']
}

adding empty lines to a JS file and staging it, running node ./index.js will remove the file from modified and staged, because npm run lint:base --fix will basically just remove the lines.

Also, adding console.log("test"); to a a file, staging it, and running node ./index.js, will leave an console.log('test') edit to staged.

EDIT: Regarding relative, I might add a --relative flag. But that's basically just moving the deprecated advanced configuration to command line flags.

@Kocal

This comment has been minimized.

Copy link
Author

commented Jul 2, 2019

Hum weird, I will take some time this evening to investigate 🧐

Regarding relative, I might add a --relative flag. But that's basically just moving the deprecated advanced configuration to command line flags.

That would be really nice. Was there a reason to remove the advanced configuration? 🤔
When I saw this, I was like ... "well, we will stay with lint-staged 8 forever 😫" and I'm sure I'm not the only one 😄

@okonet

This comment has been minimized.

Copy link
Owner

commented Jul 2, 2019

I think it's related to relativePaths. For git add to work, pass absolute paths to it that you're getting in the function arguments.

@iiroj

This comment has been minimized.

Copy link
Collaborator

commented Jul 2, 2019

Before adding, --relative, we should address how resolveTaskFn sets a different working directory for git commands vs the rest. Maybe if we add --relative, all the commands should run with the git repo root as the working directory.

EDIT: Or alternatively, the git commands defined in linters could also accept the actual working directory.

@iiroj iiroj referenced this issue Jul 3, 2019

Merged

fix: run all commands returned by function linters #650

0 of 2 tasks complete
@iiroj

This comment has been minimized.

Copy link
Collaborator

commented Jul 3, 2019

I think I found an issue with how tasks are created from a function linter return an array of commands. Currently, it generates a single task with multiple promises, but I refactored it to return an array of tasks instead, hopefully fixing this issue: #650

@iiroj

This comment has been minimized.

Copy link
Collaborator

commented Jul 3, 2019

Can you please re-test with lint-staged@9.0.2? It's hopefully fixed.

@Kocal

This comment has been minimized.

Copy link
Author

commented Jul 3, 2019

Thanks, it works really nice with 9.0.2! :)

I'm closing this issue since it's fixed

@Kocal Kocal closed this Jul 3, 2019

@Kocal Kocal changed the title Files altered by linting tools are not commited anymore in v9 Files altered by linting tools are not commited anymore in v9.0.0 Jul 3, 2019

@Kocal Kocal changed the title Files altered by linting tools are not commited anymore in v9.0.0 Files altered by linting tools are not commited anymore in v9.0.1 Jul 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.