fbrctr / fabricator

A tool for building website UI toolkits and style guides
http://fbrctr.github.io/
MIT License
1.11k stars 124 forks source link

Husky pre-commit hook fails due to invalid hook value #310

Closed sethlopez-dynamit closed 5 years ago

sethlopez-dynamit commented 5 years ago

Expected behavior

Starting with a default fabricator instance, I should be able to create a git repository in the fabricator directory and commit the fabricator files successfully.

Current behavior

Starting with a default fabricator instance, when attempting to commit the fabricator files to a git repository that's been created in the fabricator directory, the husky pre-commit hook fails with the error below.

husky > pre-commit (node v10.14.1)

Oops! Something went wrong! :(

ESLint: 5.9.0.
No files matching the pattern ".,git" were found.
Please check for typing mistakes in the pattern.

husky > pre-commit hook failed (add --no-verify to bypass)

As you can see from the error, the pre-commit hook command is malformed.

Possible solution

One solution would be to fix the invalid pre-commit hook value by either concatenating both commands in the hook into a single string.

  // changing this
  "husky": {
    "hooks": {
      "pre-commit": [
        "eslint --fix .",
        "git add ."
      ]
    }
  }

  // to this
  "husky": {
    "hooks": {
      "pre-commit": "eslint --fix . && git add ."
    }
  }

Another possible solution is to remove the pre-commit hook altogether. I propose this solution because not everyone wants to lint and fix before committing their changes to their repo. Removing the pre-commit hook would save others from having to remove it themselves. Additionally, if someone wants this in their project, they can add it themselves.

Details

Fabricator: 2.0.0 Node: 10.14.1 npm: 6.4.1 OS: Mac OS 10.14 "Mojave"

ghost commented 5 years ago

I agree that the pre-commit hook it probably too strict to enforce. I'd like the project to be more generally consumable, although I think adding a default .prettierrc like we've done is fine.

If we want to ensure linting is followed as the project evolves, I think adding CI somewhere here to prevent merges until linting passed is a good move.

ghost commented 5 years ago

Adding a CircleCI integration is a free way to get there https://github.com/marketplace/circleci

ghost commented 5 years ago

I believe that travis is already configured to run on PRs. I'm not sure who set that up but we'll want access. I think this is best handled at the CI level. I'll create a PR that removes Husky.