feat: introduce eslint with typescript-eslint to replace tslint #2492

raymondfeng · 2019-02-26T22:53:27Z

See some background at:

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

raymondfeng · 2019-02-27T05:53:47Z

See #1851

bajtos · 2019-03-01T15:22:55Z

Yay! Switching from tslint to eslint was on my personal todo list too.

Let me post my notes before I review the changes:

port linting to use renamed TS parser desktop/desktop#6684 is an example from a different project that has recently switched from tslint to eslint
can we use --cache option to speed up linting?
there are two eslint plugins to improve integration with prettier: eslint-plugin-prettier checks formatting and reports violations, eslint-config-prettier disables any eslint rules that may clash with prettier. Personally, prefer to include prettier linting in eslint run, because standalone prettier is rather slow and AFAIK it does not offer any options to cache the results (yet).
If we move prettier checks to eslint, we must still preserve Prettier checks for our Markdown documents! This utility may be useful: https://github.com/ikatyang/prettylint

bajtos

My main concern is about preserving as much of the current tslint config as possible, and not enabling any additional rules now. This pull request is large enough, it's better to not make it even larger by adding extra eslint directives like "disable ban-types".

bajtos · 2019-03-01T15:23:35Z

.vscode/settings.json

@@ -24,6 +24,15 @@
  "files.insertFinalNewline": true,
  "files.trimTrailingWhitespace": true,
  "tslint.ignoreDefinitionFiles": true,


Can we remove this tslint config too?

examples/hello-world/package.json

packages/cli/generators/openapi/spec-helper.js

bajtos · 2019-03-01T15:29:23Z

packages/cli/generators/project/templates/.eslintrc.js.ejs

+    node: true,
+  },
+  extends: ['eslint:recommended', 'plugin:@typescript-eslint/recommended'],
+  rules: {


Please don't duplicate the content of @loopback/eslint-config here, it will quickly get out of sync.

IMO, we should always extend from @loopback/eslint-config, it's a shared config that's can be used independently from @loopback/build.

Please don't duplicate the content of @loopback/eslint-config here, it will quickly get out of sync.

IMO, we should always extend from @loopback/eslint-config, it's a shared config that's can be used independently from @loopback/build.

☝️ This is not addressed yet AFAICT.

packages/eslint-config/index.js

packages/eslint-config/.eslintrc.js

packages/metadata/src/__tests__/unit/decorator-factory.unit.ts

packages/metadata/src/inspector.ts

packages/metadata/src/decorator-factory.ts

packages/eslint-config/.eslintrc.js

benchmark/src/__tests__/benchmark.integration.ts

packages/metadata/src/__tests__/unit/decorator-factory.unit.ts

packages/context/src/binding.ts

packages/context/src/binding-key.ts

packages/context/src/context.ts

packages/context/src/inject.ts

packages/http-caching-proxy/src/__tests__/integration/http-caching-proxy.integration.ts

bajtos · 2019-03-08T09:34:01Z

packages/eslint-config/.eslintrc.js

+    'comma-dangle': ['error', 'always-multiline'],
+    'no-mixed-operators': 'error',
+    'no-console': 'off',
+    'no-undef': 'off',


What is this rule about and why are we turning it off, against the recommendations?

bajtos · 2019-03-08T09:34:06Z

packages/eslint-config/.eslintrc.js

+    'no-mixed-operators': 'error',
+    'no-console': 'off',
+    'no-undef': 'off',
+    'no-inner-declarations': 'off',


What is this rule about and why are we turning it off, against the recommendations?

bajtos · 2019-03-08T09:34:12Z

packages/eslint-config/.eslintrc.js

+    'no-inner-declarations': 'off',
+    // TypeScript allows method overloading
+    'no-dupe-class-members': 'off',
+    'no-useless-escape': 'off',


What is this rule about and why are we turning it off, against the recommendations?

bajtos · 2019-03-08T09:37:03Z

packages/eslint-config/.eslintrc.js

+    '@typescript-eslint/no-parameter-properties': 'off',
+    '@typescript-eslint/no-angle-bracket-type-assertion': 'off',
+    '@typescript-eslint/prefer-interface': 'off',
+    '@typescript-eslint/no-namespace': 'off',


I see a bunch of rules that we are disabling against the recommendations. What is your reasoning for disabling them? Also why is ban-types treated differently from these rules? Is it because fixing ban-types violations is easier than fixing violations of these other rules? That seems to be a poor excuse to me.

IMO, we should add ban-type to the list of disabled rules to keep this PR smaller and then incrementally try to enable most of the recommended rules, one by one. Starting with ban-types if you like.

packages/eslint-config/.eslintrc.js

bajtos · 2019-03-08T09:40:52Z

@raymondfeng I would love to see this pull request landed soon, but unfortunately it seems to make too many changes that are very loosely related (if related at all). Can you find a way how to reduce the size of this patch so that we can land it sooner?

I like the idea of inheriting all recommended rules. For this first iteration, I prefer to disable any of these recommendations that we are not already complying with, to avoid large code changes and/or adding /* eslint-disable */ statements that we probably should want to remove in the near future anyways.

raymondfeng · 2019-03-11T18:33:40Z

@bajtos Let's start with #2572.

bajtos

I am looking forward to see this change landed 🕺

Please address my comments below.

EDIT: Some of the comments are collapsed by GitHub, please make sure to review them all!

bajtos · 2019-05-31T08:38:49Z

.vscode/settings.json

+  "eslint.validate": [
+    "javascript",
+    {
+        "language": "typescript",


I find this suspicious. Could you please explain what does it mean to validate javascript files using typescript language?

See https://marketplace.visualstudio.com/items?itemName=dbaeumer.vscode-eslint. The setting is an array. It basically says javascript is validated and typescript is validated and auto-fixed.

Fair enough. Can you capture this in a code comment please? I believe settings.json allow comments.

examples/todo-list/src/__tests__/unit/controllers/todo-list-todo.controller.unit.ts

packages/booter-lb3app/fixtures/lb3app/common/models/coffee-shop.js

packages/build/test/integration/scripts.integration.js

packages/cli/generators/model/index.js

bajtos · 2019-05-31T08:57:29Z

packages/eslint-config/eslintrc.js

+         * }
+         * ```
+         */
+        varsIgnorePattern: 'inject|(\\w+)Bindings',


I understand why we are ignoring inject, but why (\\w+)Bindings? Should we ignore config too?

Is there a way how to ensure inject is ignored only when it's @inject, but not when it's a variable name like injectFooBar?

We use @inject(RestBindings.Http.REQUEST). Both @inject and RestBindings are treated as no-used due to typescript-eslint/typescript-eslint#571.

packages/eslint-config/package.json

bajtos · 2019-05-31T09:01:50Z

packages/rest/src/__tests__/unit/coercion/utils.ts

-          {encodeValuesOnly: true},
-        );
-        url += `?${q}`;
+        {


Weird, why are we wrapping this code in a {} block?

Can you find a different solution please, one that will require less code changes?

I think it's due to https://eslint.org/docs/rules/no-case-declarations

packages/rest/src/types.ts

packages/testlab/should-as-function.d.ts

bajtos · 2019-05-31T09:12:36Z

@raymondfeng Travis CI build is failing for verify-docs step, PTAL.

https://travis-ci.org/strongloop/loopback-next/jobs/539476548

lerna bootstrap --scope @loopback/tsdocs
lerna notice cli v3.14.1
lerna info versioning independent
lerna info ci enabled
lerna notice filter including "@loopback/tsdocs"
lerna info filter [ '@loopback/tsdocs' ]
lerna WARN bootstrap Installing local packages that do not match filters from registry
lerna info Bootstrapping 1 package
lerna info Installing external dependencies
lerna ERR! npm install --no-save exited 1 in '@loopback/tsdocs'
lerna ERR! npm install --no-save stderr:
npm WARN deprecated @types/[email protected]: This is a stub types definition. 
    handlebars provides its own type definitions, so you do not need this installed.
npm ERR! code ETARGET
npm ERR! notarget No matching version found for @loopback/eslint-config@^1.0.0
npm ERR! notarget In most cases you or one of your dependencies are requesting
npm ERR! notarget a package version that doesn't exist.
npm ERR! notarget 
npm ERR! notarget It was specified as a dependency of '@loopback/tsdocs'
npm ERR! notarget

raymondfeng · 2019-05-31T16:04:51Z

Please don't duplicate the content of @loopback/eslint-config here, it will quickly get out of sync.
IMO, we should always extend from @loopback/eslint-config, it's a shared config that's can be used independently from @loopback/build.

Fixed.

bajtos

I don't have time to review the patch in all details again and unfortunately I don't see any easy way how to review only the changes made since my last review :(

I'll trust you that you have addressed all my comments well.

FWIW, LGTM.

See https://github.com/typescript-eslint/typescript-eslint

raymondfeng changed the title ~~feat: support typescript-eslint to replace tslint~~ [RFC] feat: support typescript-eslint to replace tslint Feb 26, 2019

raymondfeng force-pushed the eslint branch 4 times, most recently from f0335e9 to 8ae1074 Compare February 27, 2019 05:53

raymondfeng force-pushed the eslint branch from 8ae1074 to d0e6556 Compare February 27, 2019 16:35

raymondfeng marked this pull request as ready for review February 27, 2019 16:36

raymondfeng requested review from b-admike, bajtos, hacksparrow, jannyHou and marioestradarosa as code owners February 27, 2019 16:36

raymondfeng changed the title ~~[RFC] feat: support typescript-eslint to replace tslint~~ [RFC] feat: introduce eslint with typescript-eslint to replace tslint Feb 27, 2019

raymondfeng force-pushed the eslint branch 3 times, most recently from d9938bf to c0f474f Compare February 28, 2019 17:04

bajtos added refactor Internal Tooling Issues related to our tooling and monorepo infrastructore labels Mar 1, 2019

bajtos suggested changes Mar 1, 2019

View reviewed changes

bajtos reviewed Mar 8, 2019

View reviewed changes

raymondfeng force-pushed the eslint branch from c0f474f to 87471e5 Compare March 11, 2019 17:58

raymondfeng mentioned this pull request Mar 11, 2019

feat(build): add eslint scripts and default configs #2572

Merged

7 tasks

bajtos mentioned this pull request Mar 18, 2019

Switch from tslint to eslint #2608

Closed

bajtos mentioned this pull request May 2, 2019

feat: add lb3 example application #2803

Merged

7 tasks

raymondfeng force-pushed the eslint branch from 87471e5 to 704d4bd Compare May 29, 2019 00:32

raymondfeng force-pushed the eslint branch 4 times, most recently from db71f7f to bde28a4 Compare May 31, 2019 00:23

bajtos suggested changes May 31, 2019

View reviewed changes

raymondfeng force-pushed the eslint branch 2 times, most recently from 8fa0671 to cffd4f8 Compare May 31, 2019 16:04

raymondfeng force-pushed the eslint branch 11 times, most recently from 152cbe0 to 91db407 Compare June 1, 2019 04:42

bajtos approved these changes Jun 3, 2019

View reviewed changes

raymondfeng added 4 commits June 3, 2019 07:58

feat: replace tslint with eslint

14b5863

See https://github.com/typescript-eslint/typescript-eslint

chore: enable eslint cache

8dcbc3e

chore: report/remove unused eslint disable directives

739ee16

chore: fix eslint violations on String instead of string

cf5d97c

raymondfeng force-pushed the eslint branch from 91db407 to cf5d97c Compare June 3, 2019 14:59

raymondfeng merged commit 202b2c7 into master Jun 3, 2019

delete-merged-branch bot deleted the eslint branch June 3, 2019 15:28

bajtos mentioned this pull request Jun 4, 2019

chore: import eslint rules from eslint-config-loopback #3043

Merged

3 tasks

bajtos mentioned this pull request Sep 12, 2019

[SPIKE] switch to typescript-tslint-plugin #2026

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: introduce eslint with typescript-eslint to replace tslint #2492

feat: introduce eslint with typescript-eslint to replace tslint #2492

raymondfeng commented Feb 26, 2019 •

edited

Loading

raymondfeng commented Feb 27, 2019

bajtos commented Mar 1, 2019

bajtos left a comment

bajtos Mar 1, 2019

bajtos Mar 1, 2019

raymondfeng Mar 1, 2019

bajtos May 31, 2019

bajtos Mar 8, 2019

bajtos Mar 8, 2019

bajtos Mar 8, 2019

bajtos Mar 8, 2019

bajtos commented Mar 8, 2019

raymondfeng commented Mar 11, 2019

bajtos left a comment •

edited

Loading

bajtos May 31, 2019

raymondfeng May 31, 2019

bajtos Jun 3, 2019

bajtos May 31, 2019

raymondfeng May 31, 2019

bajtos May 31, 2019

raymondfeng May 31, 2019

bajtos commented May 31, 2019

raymondfeng commented May 31, 2019

bajtos left a comment

feat: introduce eslint with typescript-eslint to replace tslint #2492

feat: introduce eslint with typescript-eslint to replace tslint #2492

Conversation

raymondfeng commented Feb 26, 2019 • edited Loading

Checklist

raymondfeng commented Feb 27, 2019

bajtos commented Mar 1, 2019

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Mar 8, 2019

raymondfeng commented Mar 11, 2019

bajtos left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented May 31, 2019

raymondfeng commented May 31, 2019

bajtos left a comment

Choose a reason for hiding this comment

raymondfeng commented Feb 26, 2019 •

edited

Loading

bajtos left a comment •

edited

Loading