Codecov Report

Merging #173 (d6b15af) into master (e76e893) will decrease coverage by 31.01%.
The diff coverage is 57.14%.

@@             Coverage Diff             @@
##           master     #173       +/-   ##
===========================================
- Coverage   91.58%   60.56%   -31.02%     
===========================================
  Files          50        8       -42     
  Lines        1141      317      -824     
  Branches      214       50      -164     
===========================================
- Hits         1045      192      -853     
- Misses         54      104       +50     
+ Partials       42       21       -21     

The next two priorities on this (in order) are:

• DONE multi-project workspace support
• DONE automated conversion of existing tslint-disable comments to eslint-disable equivalents, leveraging tslint-to-eslint-config again wherever possible (may need some more exports here @JoshuaKGoldberg)
  • As per the other usage, I will almost certainly only need the raw conversion logic, not any kind of file resolution

For the inline comment conversion: typescript-eslint/tslint-to-eslint-config#820 -> typescript-eslint/tslint-to-eslint-config#821.

Thanks a lot for this! I just tested with v0.7.0-alpha.12, and here is some feedback as requested.

What works:

• configuration changes via the schematics
• linting via VS Code ESLint extension

What doesn't work:

• running npx ng lint is hanging forever and is transforming my computer into a jet, I had to kill the process to avoid an overheating

• it's not directly related to this PR, but as the goals of the schematics is to match the previous TSLint configuration, currently it doesn't because problems are reported as errors (so in red in VS Code) instead of warnings, which is not the current behavior of an Angular CLI project. On a more general note, this debate has happened already many times, but while some linting rules should indeed be errors, most rules are stylistic and it is very bad for developer experience to report stylistic issues the same way as real syntax errors, especially for beginners (I'm an Angular trainer, so I can testify it's a huge problem for beginners, seeing red grab their whole attention, and they focus on lint problems instead of focusing on real important things).

After checking my last point, it's more complicated than I thought: Angular TSLint configuration was indeed reporting errors on ng lint (which is a good behavior for CI), but TSLint VS Code extension was configured by default to display warnings (which is a good behavior for DX). Would be nice if the schematics could add the right configuration for the editor, but I didn't find a relevant option in ESLint. :/ Hope to find some solution, otherwise it will be an important DX regression for people learning.

@cyrilletuzi

What doesn't work:
running npx ng lint is hanging forever and is transforming my computer into a jet, I had to kill the process to avoid an overheating

Naturally you will have to provide more information about your setup than this, or it cannot be looked into. The integration-tests (because they are full Angular CLI workspaces) also provide a great resource for you to create a reproduction of the issue yourself, which will be most time efficient.

Regarding the editor behaviour, thank you for bringing that to my attention, I was not aware of that having not really used the TSLint extension in VSCode. I will take a look into the experience so that I can better understand where folks are coming from.

So after some investigation, it works on a newly generated Angular CLI app, but not on an existing app I have. I didn't manage to find what is the issue, because I checked the config was exactly the same between the 2 projects (same eslintrc config, all dependencies up to date). So it may just be about quantity of components (but the project is not very big...).

Both apps are configured with inline templates. My general config: Mac (last v10 yesterday and now v11), Node 14.15.0, all project dependencies totally up to date.

However, I was able to fix the issue by changing tsconfig.*?.json to tsconfig.json in eslintrc.json. This settings caught my eye even before having the issue, because in a normal CLI project, there is tsconfig.app.json, tsconfig.spec.json, etc. all extending tsconfig.json, so that may mean the lint is done 3 times instead of 1, and even maybe creating a recursion (thus my computer becoming a jet).

I suppose you choose tsconfig.*?.json to also support tsconfig.base.json that was tried in Angular CLI 10, then reverted but still used by NX workspaces. I think it would be better to check if the project is using tsconfig.base.json, otherwise defaulting to tsconfig.json, and to configure it with a fix value in eslintrc.json, to avoid any risk of recursion.

Thanks a lot for looking into it more - one really useful thing you can do when investigating this is invoke the command with DEBUG set for typescript-eslint (and it works for eslint itself too).

E.g.

DEBUG=typescript-eslint:* npx ng lint

You will then see what typescript-eslint is doing for each file (which programs are being built and looked up etc)

I confirm with the debug command the issue is because of tsconfig.*?.json in eslintrc.json, as it matches tsconfig.app.json and tsconfig.spec.json, but not tsconfig.json. Thus, for each file, lint logs say File did not belong to any existing programs, and thus try to update, check in deleted files, recreate a watcher program and so on, thus the computer becoming a jet in a project where there is not just one component.

2 solutions work:

• changing tsconfig.*?.json to tsconfig.json
• changing tsconfig.*?.json to tsconfig(.*)?.json, but this one still matches tsconfig.app.json and tsconfig.spec.json, creating unnecessary watchers, which may be a performance issue in big projects

Cool, thanks this has been really useful. For now, I'm just going to create a line in the sand at Angular CLI 10.1 and just say that is where support in this toolset currently starts. The schematics will now throw if they see you are using the ill-fated tsconfig.base.json. We can make this more sophisticated over time.

I have adjusted the logic to only look at the tsconfig.json (and e2e) for the root project - for any additional projects in projects/ we still need that glob pattern because there is no tsconfig.json.

@cyrilletuzi Please could you try the same project and operations with v0.7.0-alpha.13? That version also includes tslint disable comment conversion and multi-project workspace support over and above alpha.12

This is great, thank you! I just went through the process so hopefully this feedback is useful. A few minor issues I found upgrading an existing project:

1. The default .eslintrc.json looks for:

       "project": [
         "tsconfig.*?.json",
         "e2e/tsconfig.json"
       ],

   The actual project files are e2e/tsconfig.e2e.json, src/tsconfig.app.json and src/tsconfig.spec.json. Manually updating to:

       "project": [
         "src/tsconfig.*?.json",
         "e2e/tsconfig.e2e.json"
       ],

   everything seems to work as expected.

2. The rule @angular-eslint/{component,directive}-selector doesn't automatically pick up the prefix defined in angular.json, I had to manually set that too.

3. There are two projects in the root directory, and if I tried to use the script npm run ng g @angular-eslint/schematics:add-config-to-project salary-stats-e2e to add the second I got:

   Two or more projects are using identical roots. Unable to determine project using current working directory. Using default workspace project instead.
   ERROR! .eslintrc.json already exists.
   The Schematic workflow failed. See above.
   

   So I had to copy and edit the lint configuration for the -e2e project.

And one question I had at the end:

• Is it safe to uninstall TSLint and Codelyzer entirely? I subsequently did it, and all the tests still pass, but I don't know if this will cause problems in any future updates.

Commit

@textbook thanks for providing some feedback.

Is there a reason why you are deviating from the standard Angular CLI structure and file layout? Are you not running ng update to automatically keep yourself in sync with the latest? It looks like you might just be patching versions manually (eg on v11) which is not recommended.

All of your issues seem to be arising for this reason, and I don’t think it’s going to be possible to support these kinds of customisations...

I have been using ng update; as far as I'm aware I have been following the recommended upgrade processes throughout. That project started with Angular CLI 1.0.0-beta.24 and Angular 2.3.1, though, so it's possible this is an outlier!

It wasn't super difficult to figure out what was going on and fix it anyway. Might be worth showing DEBUG=typescript-eslint:* as a troubleshooting step on the migration page, as that definitely helped.

@JamesHenry

alpha.17 test:

Working:

• config issue solved, ng lint works as expected
• an application in a CLI multi-workspaces (but matches tsconfig.spec.json, not ideal for performances)

Doesn't work:

• for a library in a CLI multi-workspaces, because e2e/tsconfig.json doesn't exist in libraries; error gone if suppressing projects/xxx/e2e/tsconfig.json in .eslintrc.json

The right solution would be to detect the projectType from angular.json:

• application > ["projects/xxx/tsconfig.app.json", "projects/xxx/e2e/tsconfig.json"]
• library > ["projects/xxx/tsconfig.lib.json"]

Even better solution: each project in angular.json has a architect.build.options.tsConfig property with the correct TS config path!

Thanks folks!

@textbook - wow that's a long journey for that project! It wouldn't surprise me if the tooling let you down at some point, ng update was a little shaky at first. You can see what brand new (but v10 because done before 2 days ago) Angular CLI workspaces look like in the integration-test fixtures and how they differ in layout to your project.

@cyrilletuzi good call out, I am adding coverage for libraries.

Can you please clarify what you mean by spec not being good for performance?

Without spec you will not have a program available for linting your spec files, which means when you do run lint the createDefaultProgram logic has to kick in, which is really not good for performance. We need to include spec in the patterns in all cases to cover all .ts files

Even better solution: each project in angular.json has a architect.build.options.tsConfig property with the correct TS config path!

Again this is correctly describing doing less but it therefore isn't doing enough - we can't just set the source code files up for linting, we need to set all relevant files up

With tsconfig.lib/app.json (and no tsconfig.spec.json) as project in my library/application's .eslintrc.json, .spec.ts files are already linted.

@JamesHenry You're right, without tsconfig.spec.ts, spec files are linted but via a default program.

However, the glob expression is also catching tsconfig.lib.prod.json. So a fixed tsconfig.lib.json and tsconfig.spec.json may be better for librairies.

Ok ha I should have tested the .prod. one - I was lazily just hoping it wouldn't catch it. I'll make it an explicit lib and spec

And a fixed tsconfig.app.json and tsconfig.spec.json for applications would also be better after some thinking. I just remembered that for example in one of my Angular lib, I had to do a tsconfig.es5.json file for CI tests in Internet Explorer, then the glob expression would also match that while it shouldn't. All other TS config files will probably be environment files and should not be taken into account when linting.

Yeah sounds good, I'll make everything explicit

Published 0.7.0-alpha.14

Migration is working well with alpha.14, thanks a lot @JamesHenry!

Now that configuration is OK, I would like to discuss another topic: while all my projets were passing lint, after the migration, there are a lot of lint errors (and it's just small projects). It means that the default rules of @angular-eslint are not the same than the current Angular CLI ones.

I've identified several causes:

1. templates (including inline templates) are now fully linted, which was not the case with Angular CLI (for example, I have a lot of This line has a length of xxx. Maximum allowed is 140 reports happening in inline templates) > this is actually a good change, but it should be mentioned in documentation so users are not surprised

2. @angular-eslint/recommended has a different configuration from Angular CLI and has some opinionated (and debatable) choices. It is an important issue because:

• existing projects won't pass lint, with thousands of errors in big projects, so this repo will be overwhelmed by issues where people will complain about that,
• people will argue about the default choices (starting by me: I'm a JavaScript/Angular trainer, so I have a quite good idea of best practices, and I totally disagree with some things the lint is currently asking me to change).

I think an important discussion should happen here, on multiple topics:

• default rules: do we stick to the current Angular CLI ones, or is this the opportunity for a change?
• governance: who decides of the previous point? the Angular team as before, or is this the opportunity for the community to be in charge for once?

I have my own opinion, but I'm not sure if you want to discuss about that here or in another issue. While technically it can be separated, for your own sake, I really think you should not ship the migration schematics before this topics are resolved, otherwise the repo will for sure be overwhelmed by issues.

Hmm this is surprising because as of this PR, @angular-eslint/recommended is entirely based on the converted OOTB tslint.json from the Angular CLI.

Please could you provide the tslint.json you are referring to and what ESLint you are ending up with?

It is also not expected that max-len would apply in templates based on the recommended config from the template plugin, so again something seems to be misconfigured there vs what is intended by the toolset...

There's a lot of important work in this PR so I'm merging it now and turning it into 0.7.0-beta.0 on latest tag.

We can make further improvements and tweaks in follow up PRs