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

WIP: Switch from tslint to eslint #527

Open
wants to merge 2 commits into
base: master
from

Conversation

@BBosman
Copy link
Member

commented Jul 17, 2019

Pull Request

📖 Description

The people behind tslint recently announced that they're going to deprecate it in favour of eslint (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):

  • Updated all package.json files. Dropped the tslint related packages and added eslint. I also kept the sonarjs stuff, but did switch to its eslint counterpart.
  • __tests__ has a modified ruleset. It disables rules we don't want enforced within test code and adds cypress and mocha support.
  • eslint does not support rational comments after eslint-disable* statements, so I moved those to the line to which the disable applies.
  • eslint does not have the concept of defaultSeverity, 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

  • Decide on a ruleset.
  • Fix violations.
  • Remove the remaining tslint files and references.
  • See if we can lower the burden of conforming by autofixing stuff (or something like that).
@codeclimate

This comment has been minimized.

Copy link

commented Jul 17, 2019

Code Climate has analyzed commit 848c08a and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

The test coverage on the diff in this pull request is 64.2% (50% is the threshold).

This pull request will bring the total coverage in the repository to 76.2% (0.0% change).

View more on Code Climate.

@baerrach

This comment has been minimized.

Copy link

commented Jul 18, 2019

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.
The first step then is to fix the eslint errors before making any other code changes.
This way moves the cost of change into the feature being implemented so that it pays the "lint tax" rather than having a zero value task of updating all lint files.

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.

@baerrach

This comment has been minimized.

Copy link

commented Jul 18, 2019

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

    invalid
      √ // filename=/some/dir/webpack.config.js
  module.exports = function () {
    return {
      entry: {
        app: ['not-aurelia-bootstrapper']
      },
    }
  };

If this stuff is useful it might be worth putting into the official repository.

@fkleuver

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

@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.
But I'm not sure. Perhaps it does make sense to do certain things with a linter instead. You gave webpack.config entry point as an example, do you have any other examples?

@baerrach

This comment has been minimized.

Copy link

commented Jul 18, 2019

@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 use.globalResources and setRoot, but I haven't tried them yet on real code in eslint.

@fkleuver

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

@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.
Meaning that the logic that does all this will be capable of a lot more than just pre-compiling if used properly.
Hooking it into vs code, we could have, for example:

  • Aurelia-specific intellisense / autocompletion (e.g. after an opening < in html, you'll get a dropdown with what custom elements are available, or after opening a ${, you'll see available view model properties, etc)
  • Aurelia-specific syntax highlighting
  • VS Code shortcuts for auto refactoring a component or generating a new custom element
  • Providing suggestions/warnings of things that might go wrong at runtime (e.g. trying to use a custom element that is not registered, or writing an invalid binding)

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.

@baerrach

This comment has been minimized.

Copy link

commented Jul 19, 2019

@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

@fkleuver

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

@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 :)

@baerrach

This comment has been minimized.

Copy link

commented Jul 22, 2019

I've managed to hack onto the eslint rules to get webpack.config.js and aurelia PLATFORM.module() wrapping to get checked, see https://github.com/baerrach/eslint-plugin-aurelia/tree/feat-rule-platform-modulename

@3cp

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

I don't think we have PLATFORM.module() or PLATFORM.moduleName() in vnext.

@BBosman BBosman force-pushed the eslint branch from d50cb15 to 07d6d70 Jul 22, 2019

@fkleuver

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

No, we don't have PLATFORM.moduleName anymore. It's no longer needed :)

@codecov

This comment has been minimized.

Copy link

commented Jul 22, 2019

Codecov Report

Merging #527 into master will decrease coverage by 1.32%.
The diff coverage is 65.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #527      +/-   ##
==========================================
- Coverage   76.83%   75.51%   -1.33%     
==========================================
  Files         144      129      -15     
  Lines       12934    12259     -675     
  Branches     2453     2285     -168     
==========================================
- Hits         9938     9257     -681     
- Misses       2996     3002       +6
Impacted Files Coverage Δ
packages/runtime-html/src/binding/listener.ts 2% <ø> (ø) ⬆️
packages/jit/src/expression-parser.ts 100% <ø> (ø) ⬆️
packages/router/src/router.ts 85.54% <ø> (-0.61%) ⬇️
packages/runtime/src/definitions.ts 88.88% <ø> (ø) ⬆️
packages/runtime/src/observation/array-observer.ts 98.2% <ø> (ø) ⬆️
packages/debug/src/binding/unparser.ts 5.57% <ø> (ø) ⬆️
packages/runtime/src/binding/ast.ts 84.77% <ø> (-0.07%) ⬇️
...es/runtime/src/resources/custom-attributes/with.ts 97.22% <ø> (ø) ⬆️
packages/router/src/queue.ts 91.52% <ø> (ø) ⬆️
...ime/src/resources/custom-attributes/replaceable.ts 100% <ø> (ø) ⬆️
... and 98 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c65ccf0...3f36c11. Read the comment docs.

@zewa666

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

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?

@BBosman

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2019

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?

@zewa666 I already did something like that in __tests__. It has it's own .eslintrc.js, which inherits our default .eslintrc.js, but adds cypress and mocha plugins/rules which are specific to that package and disables some rules from the base set which don't make sense there.

@BBosman BBosman force-pushed the eslint branch from 07d6d70 to 848c08a Jul 24, 2019

@baerrach

This comment has been minimized.

Copy link

commented Jul 24, 2019

What about things like code smells, or patterns/anti-patterns?
Are these documented somewhere?
Could they be encoded in tooling?

@fkleuver

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

What about things like code smells, or patterns/anti-patterns?
Are these documented somewhere?

Not really documented, but we will work on this. @EisenbergEffect maybe we should have a special section for this in the documentation?

Could they be encoded in tooling?

Yep. Some examples that come to mind for vNext:

  • Inject interfaces instead of concrete classes.
  • Use the ILifecycle component for deterministically deferring work instead of setTimeout or requestAnimationFrame. This ensures work is correctly prioritized.
  • Use the binding hook instead of attaching or attached for initializing data (ensures the DOM is in up-to-date state immediately when it is mounted, instead of mounting first and then mutating it again, which may slow down initial start)
  • Do not directly reference the document or window globals, use DOM or the injectable IDOM instead (keeps your app easy to test and SSR-compatible)
  • Warn when referencing variables in your HTML template that do not exist in your view model.
  • Warn when using a custom element/attribute in your HTML that is not registered as a dependency.
  • Warn when mutating an array in a way that cannot be detected by change detection with the current observation mechanism. (for example, you need to enable proxy observation if you want to directly assign indices or change the length)
  • Warn when excessively zigzagging observed values throughout multiple components, as this makes them hard to test (prefer using a centralized store or the EventAggregator if many components need to synchronize certain values)

These are just off the top of my head

@baerrach

This comment has been minimized.

Copy link

commented Jul 25, 2019

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.
I had to search for it because I couldn't find it.
I'd pull it up a level.

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.

@fkleuver

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

@baerrach What exactly are you doing? Is your code using v1 or v2 of Aurelia? The best practices mentioned above apply to v2.

@baerrach

This comment has been minimized.

Copy link

commented Jul 25, 2019

@fkleuver I'm using v1, but a lot of those rules apply to that too.

@BBosman BBosman force-pushed the eslint branch from 848c08a to 3f36c11 Aug 19, 2019

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