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

Add ESLint #5259

Merged
merged 6 commits into from
Mar 27, 2020
Merged

Add ESLint #5259

merged 6 commits into from
Mar 27, 2020

Conversation

UnsolvedCypher
Copy link
Contributor

Closes #1924 by adding ESLint to overcommit. It auto-fixes several hundred problems but more issues remain. You can run 'yarn run eslint app/javascript` to see remaining problems and warnings.

@UnsolvedCypher UnsolvedCypher changed the title Eslint Add ESLint Mar 12, 2020
@viroulep
Copy link
Contributor

That's awesome, thanks @UnsolvedCypher!
I've started to take a look to these remaining hundreds of issues, some of them are not the most straightforward to fix (and some of them are because of our currently awkward double js assets pipeline...).

May I push to your branch to commit the necessary fixes?
(I'll probably force-push to first fix the conflicts so you may need to reset --hard your local branch once it's done :s)
Github gives me the technical possibility to push there, but I'd rather ask first.

@UnsolvedCypher
Copy link
Contributor Author

Sure, please feel free to push! If you want, I can re-create this commit from the most recent repo version (all that needs to be done is adding the dependencies to yarn, overcommit config and then run yarn run eslint app/javascript) just so that it's easier to merge later.

If the errors that are unfixable are all a certain class of error (like import errors), we can just turn that entire category of errors off for now.

@viroulep
Copy link
Contributor

If you want, I can re-create this commit from the most recent repo version (all that needs to be done is adding the dependencies to yarn, overcommit config and then run yarn run eslint app/javascript) just so that it's easier to merge later.

That's exactly what I did :)
The only conflicts are due to our dependencies being updated frequently, the actual diff on the code works fine!

If the errors that are unfixable are all a certain class of error (like import errors), we can just turn that entire category of errors off for now.

Yes definitely, it's good to try migrating some of our stuff from the assets pipelines if we can and it fixes some detected errors ^^
I'll consider ignoring some class of errors as our last resorts when fixing them would be impossible or too "dirty".

@viroulep
Copy link
Contributor

Finally.
I'm not sure which was worse, going through all these errors or taking a look back at my javascript from 2 years ago...

@viroulep
Copy link
Contributor

Damn that's definitely non intuitive, but overcommit passes all the files not excluded in the configuration after the "app/javascript" path, so it validates also all our sprockets assets (for which I'm fine just using jshint, since we can't even use ES in there...).
This is clever when doing pre-commit since it only validates files which changed, but it's not the same behavior as when running the command manually.

Also it doesn't go over jsx file by just running on the directory, because eslint uses only js by default (it's somewhere in the user guide, but not in the help text).
The command to validate everything pretty much like overcommit is:

bin/yarn run eslint app/javascript/ -f compact -c .eslintrc.json --ext .js --ext .jsx

There are actually 400+ more issues to fix in our jsx files!

I think I'll end up adding something like this in the overcommit config:

  EsLint:
    required_executable: 'WcaOnRails/bin/yarn'
    enabled: true
    include:
      - 'WcaOnRails/app/javascript/**/*.js[x]'
    command: ['WcaOnRails/bin/yarn', 'run', 'eslint', '-c', '.eslintrc.json', '-f', 'compact']

(so that it's only overcommit which decides what goes after that, and we manually glob only the file we want to pass to it)

I'll also disable JsHint on our webpacker assets, because some validations conflicts with EsLint :p

@viroulep
Copy link
Contributor

viroulep commented Mar 27, 2020

I spent a whole lot of time fixing the edit-schedule pack, I've extensively manually tested it but hopefully I didn't break it.
I'll just add an exception for the edit-events one, it's basically legacy code and I'm fine with it not being fully "clean".
New packs and utils should definitely be clean though ^^

I'll make sure tests pass and then sqash/reorganize commits so that if something break we can cleanly revert the edit-schedule one.

@UnsolvedCypher
Copy link
Contributor Author

Wow, that is an amazing amount of work! And sorry I completely forgot about enabling scanning for jsx 🤦‍♂️

@viroulep
Copy link
Contributor

Thanks again!
I wish we had that from the beginning ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set up linting and style checking for javascript
2 participants