Switch from tslint to eslint #527
Conversation
|
Code Climate has analyzed commit bf980d4 and detected 3 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 78.5% (50% is the threshold). This pull request will bring the total coverage in the repository to 80.1% (0.0% change). View more on Code Climate. |
|
I need to be able to trust that any linting errors are things that need to get fixed and should always be at zero within the codebase, otherwise they are noise you train yourself to ignore. This make it difficult to change rulesets and then slowly migrate the code to conform to those rules because that trust is lost. An option is to do this iteratively and only change files when they get touched for some other reason. You can also run the autofixer over the code base to get the 80/20 effort sorted. It might be best to fix one rule at a time so that you can visually inspect the changes for correctness. |
|
Another thing to consider is whether it is worth writing your own rules to help users of Aurelia. https://github.com/bryanrsmith/eslint-plugin-aurelia has some rules. I've started experimenting with eslint rules to see if I could write ones to check for the errors listed for webpack: https://aurelia.io/docs/build-systems/webpack/a-basic-example#introduction I've got a skeleton working that will check the entry point, here's the rule test case If this stuff is useful it might be worth putting into the official repository. |
|
@baerrach I like the idea of Aurelia lint rules, though I'm hesitant on actually implementing them with a linter since in vNext we will have AOT performing deep project analysis and reporting various issues. |
|
@fkleuver What's AOT? This one https://aurelia.io/docs/build-systems/webpack/a-basic-example#platformmodulename has a bunch of useful things that need to be tested. I've got eslint rules built and tested for |
|
@baerrach Ahead-Of-Time (AOT) compilation is the concept of pre-compiling and optimizing the application during the build/bundle process. So conceptually it's fairly specific to "producing a smaller bundle that performs better at runtime", but we're going to be pretty ambitious with this.
So there's a lot of overlap there with what a linter normally does. Of course the main culprit in my case is the required VS Code integration. Uncovering all this info programmatically is one thing, passing it to VS Code so that users gain something from it, is another. |
|
@fkleuver You will be unhappy to hear I'm an emacs user :p If this can be integrated into https://microsoft.github.io/language-server-protocol/ that would be a better win than being specific with an IDE. Admittedly there are grumbles in the emacs community that using LSP dilutes what emacs offers but at least I can access it when someone else writes the elisp code to use it ala https://github.com/emacs-lsp/lsp-mode |
|
@baerrach That's good to know. In any case, the vast majority of stuff will be in AOT package which is not coupled to any IDE, but doing the language server integration is definitely a good idea. Haven't quite looked into that yet :) |
|
I've managed to hack onto the eslint rules to get |
|
I don't think we have |
|
No, we don't have |
Codecov Report
@@ Coverage Diff @@
## master #527 +/- ##
=======================================
Coverage 79.61% 79.61%
=======================================
Files 161 161
Lines 13725 13725
Branches 2666 2666
=======================================
Hits 10927 10927
Misses 2798 2798
Continue to review full report at Codecov.
|
|
What about plugin specific lint rules? E.g the Store plugin makes use of rxjs. Would we extend the eslint config on a package basis or have everything in one set? Also what about custom rules. Per package or for everything? |
@zewa666 I already did something like that in |
|
What about things like code smells, or patterns/anti-patterns? |
Not really documented, but we will work on this. @EisenbergEffect maybe we should have a special section for this in the documentation?
Yep. Some examples that come to mind for vNext:
These are just off the top of my head |
|
I had my first attempt at modifying our code base today. Even after all my reading I still ran into about 10+ n00b errors. And trouble shooting them was a PITA! I only managed to get things working at knock off time, so I'll revisit it tomorrow to see if I can recreate my issues and work out how best to avoid them for next time. Also, the current documentation layout lends itself well to front-to-back reading, but not very well when you want to look up something. Search is great, when you can remember what you are looking for. An example, Expression Syntax is 3 clicks, Guides > Binding > Basics > Expression Syntax. And I'd probably rework the navigation, I'd rather a tree I can expand as the depth sliding makes it difficult to quickly click around. |
|
@baerrach What exactly are you doing? Is your code using v1 or v2 of Aurelia? The best practices mentioned above apply to v2. |
|
@fkleuver I'm using v1, but a lot of those rules apply to that too. |
563e9b8 to
bf980d4
Compare
|
So this is now in a mergeable state imho. It's a "plain" conversion from It results in a "huge" list of violations, which I've marked as The failing |
|
Very nice, thank you! I'll merge this right away to reduce merge conflict risks |
Pull Request
📖 Description
The people behind
tslintrecently announced that they're going to deprecate it in favour ofeslint(source), which is related to the fact that the TypeScript team is adopting as their preferred linting platform for TypeScript (source). It might be worthwhile to make the switch in the vNext repo as well, to ensure we have a future proof linting solution.Related is a recent discussion on aurelia/tools#16 (comment) between @baerrach and @EisenbergEffect about linting in general, but also on why our ruleset is set as it is and about recommended linting rules/settings for vNext skeletons.
That's the reason I started this PR. To see how much effort switching would take, but also to kick off a discussion about our view on linting in general.
🎫 Issues
👩💻 Reviewer Notes
Noteworthy changes (for now):
package.jsonfiles. Dropped thetslintrelated packages and addedeslint. I also kept the plugins likesonarjsand others, but did switch to theireslintcounterparts.__tests__has a modified ruleset. It disables rules we don't want enforced within test code and addscypressandmochasupport.eslintdoes not support rational comments aftereslint-disable*statements, so I moved those to the line to which the disable applies.eslintdoes not have the concept ofdefaultSeverity, so every rule where you want to diverge from the default of that rule, you have to do that for each rule separately.Deciding which rules to enable/disable is still an open discussion. Also, fixing some of the rules (quote style, indentation, ...) result in huge changesets, so those are better split off into separate PR's.
📑 Test Plan
CircleCI (I expect linting to work there, but give a huge amount of violations (as warnings))
⏭ Next Steps