refactor(docs-infra): migrate from tslint to eslint #42820

dario-piotrowicz · 2021-07-11T09:29:20Z

migration to eslint as tslint has been deprecated

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

ng lint runs the deprecated tslint

Issue Number: N/A

What is the new behavior?

ng lint runs eslint

Does this PR introduce a breaking change?

Yes
No

dario-piotrowicz · 2021-07-11T09:42:32Z

Hi @gkalpak 🙂

So, the migration is looking.... good?... hopefully? 😅

I opened this draft PR so start a bit of a discussion and see how you see this, I hope you don't mind 🙂

dario-piotrowicz · 2021-07-11T09:43:47Z

First of all, what do you think of the incompatible typescript version? is this something we can accept or it's a deal breaker? 🤔

(I would not want to downgrade the current typescript just to accommodate eslint)

aio/.eslintrc.json

aio/src/404-body.html

aio/src/app/app.component.spec.ts

aio/src/app/shared/search-results/search-results.component.ts

aio/src/app/custom-elements/resource/resource-list.component.html

aio/src/app/custom-elements/resource/resource-list.component.ts

aio/src/app/shared/search-results/search-results.component.ts

aio/.eslintrc.json

gkalpak

Awesome work (as usual), @dario-piotrowicz ❤️
Just a few minor comments, but this is looking very promising 😃

aio/angular.json

aio/package.json

aio/src/app/custom-elements/resource/resource-list.component.html

aio/.eslintrc.json

dario-piotrowicz · 2021-07-13T21:42:33Z

Awesome work (as usual), @dario-piotrowicz
Just a few minor comments, but this is looking very promising

Thanks a lot for the kind words!!!! 🙂

Well, your comments don't seem to be "just a few" 😅 (I'm going to check them out now!),
I really hope I am not burdening you with this 😓
thank you so very much for taking the time to review the PR 🥰

By the way, could you comment on the typescript version warning? don't you see it as a problem here? 🙂

aio/.eslintrc.json

gkalpak

Thx for making the changes, @dario-piotrowicz 👍
Getting closer 🏃‍♂️

Well, your comments don't seem to be "just a few" 😅

Given the size of the PR, they were really "just a few" 😛

I really hope I am not burdening you with this 😓
thank you so very much for taking the time to review the PR 🥰

It would have taken me much more time to do the actual work myself, so thank you for taking it on 💪

By the way, could you comment on the typescript version warning? don't you see it as a problem here? 🙂

Sorry, I meant to comment on that, but then forgot.
Yeah, it's not ideal, but I don't think it is a problem here (as long as linting passes 😅).

_{[NOTE-TO-SELF] Unresolved comments from previous review:}
_{#42820 (comment), #42820 (comment), #42820 (comment), #42820 (comment), #42820 (comment), #42820 (comment)}

aio/.eslintrc.json

dario-piotrowicz · 2021-07-14T22:22:14Z

@gkalpak I think that it's looking good 😃

I was about to put this as ready for review but I noticed that the test_aio is failing and it is because of the example-lint and tools-lint scripts which don't work anymore as they use tslint which has been removed as part of the migration

I did not notice those before (and I am not sure what they are about), I will have a look tomorrow, hopefully once these are taken care of we will be in a good place with this 🙂

gkalpak · 2021-07-15T12:03:06Z

I was about to put this as ready for review but I noticed that the test_aio is failing and it is because of the example-lint and tools-lint scripts which don't work anymore as they use tslint which has been removed as part of the migration

Ah, good point, @dario-piotrowicz. For now, I think it is better to just focus on migrating the AIO app to eslint and keep tslint for the other scripts that need it (to keep the PR scope manageable). In a follow-up PR we can look into switching to eslint for more AIO scripts.

dario-piotrowicz · 2021-07-16T14:32:36Z

Looks awesome
Thank you so much for taking care of this, @dario-piotrowicz

And the detailed commit message is super helpful too

One last question regarding:

for some reason the "no-console" rule would not work in eslint and has been substituted with an equivalent "no-restricted-syntax" one

What exactly didn't work with the no-console rule? (I've never had problems with it in other projects )

Reviewed-for: global-docs-approvers

thanks!!!!! 😍

about the "no-console", sorry that's just another silly mistake of mine 🙈 😅

I just checked and it does work, the issue now is that the eslint rule has the allows option in which you specify which methods you want to allow, whilst the tslint rule made you specify the ones that you didn't want to allow

so with no-restricted-syntax I am keeping the same exact behaviour here, if I had to do the same with no-console I think I'd have to specify quite some methods 😢 (https://developer.mozilla.org/en-US/docs/Web/API/Console#methods)

Alternatively I can just put no-console and only allow log, warn and error, that should also be pretty similar I think

what do you think the best solution here would be? 🙂

( once you let me know I'll update the commit message and eventually change the rules 🙂 )

gkalpak

Cool! I think enabling the no-console rule and only allowing log, warn and error is fine. (We could also consider info and debug, but they are not used anywhere, we can leave them out for now.)

dario-piotrowicz · 2021-07-19T18:16:21Z

@gkalpak should be all good now 😃

gkalpak

💯 🚀

aio/.eslintrc.json

migrate aio to eslint as tslint has been deprecated, the migration is restricted to the aio app and its e2e tests and does not include the other tools, for such reason both tslint and codelyzer have not been removed (to be done in a next PR) some minor tweaks needed to be applied to the code so that it would adhere to the new ESLinting behaviour most TSLint rules have been substituted with their ESLint equivalent, with some exceptions: * [whitespace] does not have an ESLint equivalent (suggested to be handled by prettier) * [import-spacing] does not have an ESLint equivalent (suggested to be handled by prettier) * [ban] replaced with [no-restricted-syntax] as there is no (official/included) ESLint equivalent some rules have minor different behaviours compared to their TSLint counterparts: * @typescript-eslint/naming-convention: - typescript-eslint does not enforce uppercase for const only. * @typescript-eslint/no-unused-expressions: - The TSLint optional config "allow-new" is the default ESLint behavior and will no longer be ignored. * arrow-body-style: - ESLint will throw an error if the function body is multiline yet has a one-line return on it. * eqeqeq: - Option "smart" allows for comparing two literal values, evaluating the value of typeof and null comparisons. * no-console: - Custom console methods, if they exist, will no longer be allowed. * no-invalid-this: - Functions in methods will no longer be ignored. * no-underscore-dangle: - Leading and trailing underscores (_) on identifiers will now be ignored. * prefer-arrow/prefer-arrow-functions: - ESLint does not support allowing standalone function declarations. - ESLint does not support allowing named functions defined with the function keyword. * space-before-function-paren: - Option "constructor" is not supported by ESLint. - Option "method" is not supported by ESLint. additional notes: * the current typescript version used by the aio app is 4.3.5, which is not supported by typescript-eslint (the supported versions are >=3.3.1 and <4.3.0). this causes a warning message to appear during linting, this issue should likely/hopefully disappear in the future as typescript-eslint catches up * The new "no-console" rule is not completely equivalent to what we had prior the migration, this is because TSLint's "no-console" rule let you specify the methods you did not want to allow, whilst ESLint's "no-console" lets you specify the methods that you do want to allow, so and in order not to have a very long list of methods in the ESLint rule it's been decided for the time being to simply only allow the "log", "warn" and "error" methods * 4 dependencies have been added as they have been considered necessary (see: angular#42820 (comment)) extra: * the migration has been performed by following: https://github.com/angular-eslint/angular-eslint#migrating-an-angular-cli-project-from-codelyzer-and-tslin * more on typescript-eslint at: https://github.com/typescript-eslint/typescript-eslint

alxhub · 2021-07-20T18:55:02Z

I switched this to target: minor because it doesn't merge cleanly into the current patch branch (12.1.x). Given the nature of this change, I think this is probably fine.

angular-automatic-lock-bot · 2021-08-20T16:11:00Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

google-cla bot added the cla: yes label Jul 11, 2021