Proposal: Add ESLint, git pre-hooks

[theajr]

Hello @jakoblind

To maintain consistent coding standards in this repository, we should add the following.

• [x] eslint
• [x] babel-eslint
• [x] lint-staged
• [x] husky

Please have a look and let me know your thoughts.

[jakoblind]

Good idea, I agree. I'm not sure we need both lint-staged and husky. Isn't one of them enough?

We also need to agree on the eslint rules. I suggest starting with something not too strict, and then add rules continuously.

[theajr]

@jakoblind In most of the cases lint-staged and husky are being used together. I will try to analyse more and update you.

And coming to eslint rules, my choice would be using eslint:recommended, airbnb, plugin:prettier/recommended and prettier/react.

[jakoblind]

I'll keep this issue open because there are some remaining tasks related to cleaning up the code. I quote @theajr from his merged PR:

here are 240+ error altogether, for now I updated eslint rules such that they will be ignored. We should fix them some at a time. Below are the high priority ones and having more errors react/prop-types - having 90 errors. We should define propTypes for all components. react/destructuring-assignment - having 34 errors.

[theajr]

@jakoblind Sure, You can assign this issue to me. I am already done with adding prop-types for all components. I will raise a pull request in couple of minutes.

[jakoblind]

awesome @theajr ! :+1:

[jakoblind]

For some reason I get an error when I try to commit code now.

lind@localhost ~/d/webpack-autoconf (master)> git commit -m "test"
husky > pre-commit (node v10.15.3)
  ↓ Stashing changes... [skipped]
    → No partially staged files found...
  ❯ Running tasks...
    ❯ Running tasks for src/**/*.{js,jsx,ts,tsx}
      ✔ eslint --fix
      ✔ prettier —-write
      ✖ npm test
        git add



✖ npm test found some errors. Please fix them and try committing again.

> [email protected] test /home/jlind/dev/webpack-autoconf
> jest "/home/jlind/dev/webpack-autoconf/src/components/configurator/Features.js"

No tests found, exiting with code 1
Run with `--passWithNoTests` to exit with code 0
In /home/jlind/dev/webpack-autoconf
818 files checked.
testMatch:  - 0 matches
testPathIgnorePatterns: node_modules, .cache - 734 matches
testRegex: /.*(__tests__\/.*)|(.*(test|spec))\.jsx?$ - 9 matches
Pattern: /home/jlind/dev/webpack-autoconf/src/components/configurator/Features.js - 0 matches
npm ERR! Test failed.  See above for more details.
husky > pre-commit hook failed (add --no-verify to bypass)
jlind@localhost ~/d/webpack-autoconf (master) [1]> git status
On branch master
Your branch is up to date with 'origin/master'.

Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

	modified:   src/components/configurator/Features.js

Any ideas before I start debugging?

[theajr]

@jakoblind I faced it and fixing it now.

Root cause: in lint-staged object, we have regex to run commands on src/**/*.{js,jsx,ts,tsx} which is return all staged js, jsx, ts, and tsx files. but npm test expects *.test.js or *.spec.js, as our matching files are note test/specs, it's throwing the error.

I am trying to find the way to handle this case.

[jakoblind]

Ok I see. If it's too much work fixing, we could consider disabling running tests on commit.

[theajr]

To unblock yourself, you can move npm test to pre-push hook like shown in this screenshot.

[jakoblind]

I'm thinking maybe that's a good long term solution? To run tests on push instead. :thinking:

[theajr]

I'm thinking maybe that's a good long term solution? To run tests on push instead. 🤔

I totally agree. We can go with this approach then.

[theajr]

@jakoblind Please review the raised PR.