tslint: use a better implementation of rule checking unused variables #2156

bajtos · 2018-12-14T09:21:44Z

The built-in no-unused-variable rule has many issues.

It uses TSC API in a hacky way that breaks other rules depending on TSC API. See tslint#2736 tslint#2649 tslint#2571
Because it depends on TSC API, it cannot be checked (and auto-fixed) from inside VS Code

It is deprecated, tslint is printing the following warning now:

no-unused-variable is deprecated. Since TypeScript 2.9.
Please use the built-in compiler checks instead.

It also cannot be used with tslint as a compiler plugin.

I did an extensive search for alternatives and found no-unused from tslint-consistent-codestyle as the best alternative. See their docs to learn more about the rule.

In this pull request:

In the first commit, I am modifying our project templates to always install tslint and typescript dependencies, this is needed to satisfy requirements of @loopback/tslint-config and the new module tslint-consistent-cdestyle.
The second commit is removing no-unused-variable from tslint.build.json and configuring no-unused in tslint.common.json.
The third commit is fixing linting violations. This cannot be done as part of the previous commit because it would trigger semver-major release of all modified packages (at least I think so).

The third commit fixes configuration of VSCode tasks to use a correct problem matcher.

BREAKING CHANGES

This pull request introduces a breaking change in @loopback/tslint-config. People using comments to disable no-unused check for certain lines have to use a different directive after this change.

- // tslint:disable-next-line:no-unused-variable
+ // tslint:disable-next-line:no-unused

The new rule is also more strict and reports unused variables/parameters that are not detected by the built-in no-unused-variable rule.

cc @strongloop/loopback-next

Checklist

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

bajtos · 2018-12-14T09:23:34Z

packages/repository/test/acceptance/has-one.relation.acceptance.ts

@@ -5,7 +5,6 @@

 import {Application} from '@loopback/core';
 import {expect, toJSON} from '@loopback/testlab';
-import * as _ from 'lodash';


Unused import discovered by the new rule.

bajtos · 2018-12-14T09:24:32Z

packages/repository/test/unit/repositories/relation.repository.unit.ts

-    class testHasManyEntityCrudRepository<
-      T extends Entity,
-      ID,
-      TargetRepository extends EntityCrudRepository<T, ID>


The new rule complains about unused template parameters too. Since this test is not using ID and TargetRepository, I simplified this stub class to declare only parameters that are needed.

bajtos · 2018-12-14T09:25:40Z

packages/rest/test/acceptance/bootstrapping/rest.acceptance.ts

@@ -76,7 +76,7 @@ async function startServerCheck(app: Application) {
 }

 function sequenceHandler(
-  {request, response}: RequestContext,
+  {response}: RequestContext,


Unused parameters are reported when they are declared via destructuring.

I have mixed feelings about this change. Originally, I used {request, response} to communicate what properties are available to sequence handlers. I feel that {response} is not making that clear enough. On the other hand, the type RequestContext does contain both request & response, so I guess the "request" is still easy to discover even after this change.

On the other hand, the type RequestContext does contain both request & response, so I guess the "request" is still easy to discover even after this change.

I agree 👍

bajtos · 2018-12-14T09:27:04Z

packages/rest/test/acceptance/routing/routing.acceptance.ts

@@ -692,7 +692,7 @@ describe('Routing', () => {

    it('provides httpHandler compatible with HTTP server API', async () => {
      const app = new RestApplication();
-      app.handler(({request, response}, sequence) => response.end('hello'));
+      app.handler(({response}, sequence) => response.end('hello'));


Unused parameters are reported when they are declared via destructuring.

It's a bit harder to know that the request property is also available from the RequestContext than d14974a#r241689363.

bajtos · 2018-12-14T09:27:08Z

packages/rest/test/acceptance/sequence/sequence.acceptance.ts

@@ -44,7 +44,7 @@ describe('Sequence', () => {
  });

  it('allows users to define a custom sequence as a function', () => {
-    server.handler(({request, response}, sequence) => {
+    server.handler(({response}, sequence) => {


Unused parameters are reported when they are declared via destructuring.

bajtos · 2018-12-14T09:27:10Z

packages/rest/test/acceptance/sequence/sequence.acceptance.ts

@@ -98,7 +98,7 @@ describe('Sequence', () => {
    class MySequence implements SequenceHandler {
      constructor(@inject(SequenceActions.SEND) protected send: Send) {}

-      async handle({request, response}: RequestContext) {
+      async handle({response}: RequestContext) {


Unused parameters are reported when they are declared via destructuring.

packages/metadata/src/types.ts

bajtos · 2018-12-14T09:33:00Z

For longer term, I'd like to refactor @loopback/build and extract TSLint configuration into a new package (e.g. @loopback/tslint-config) that can be versioned independently from the rest of our build tooling.

Maybe I should do that refactoring as the first step before this pull request, to avoid a breaking change in @loopback/build?

Move tslint config to a new package
Add v1 of the new package to @loopback/build dependency
Update all packages & examples & CLI templates to load the rules from the new package, not from @loopback/build.
Replace no-unused-variable with no-unused rule, release v2 of the new package.
Update packages & examples & CLI templates to use v2. In @loopback/build, keep using v1 for backwards compatibility.

I think this is actually the best approach. Once tslint config has its own package, we can clearly communicate breaking changes in the autogenerated release notes.

bajtos · 2018-12-14T13:20:41Z

Once tslint config has its own package, we can clearly communicate breaking changes in the autogenerated release notes.

See #2159.

This PR is effectively blocked until #2159 is landed.

.vscode/tasks.json

b-admike · 2018-12-14T14:24:56Z

Nitpick on first commit message:

The 3rd-party rule "no-unused" works does not require TSC API and thus
can be used checked from the VS Code too.

it looks like works doesn't belong in this sentence.

Maybe I should do that refactoring as the first step before this pull request, to avoid a breaking change in @loopback/build?

That sounds great too.

bajtos · 2018-12-14T17:06:39Z

Rebased on top of the new master. I need to fix the way how we are installing tslint to make linting work again, and update the commit message to clearly describe the breaking changes & migration guide for release notes.

packages/metadata/src/types.ts

bajtos · 2019-01-03T09:50:11Z

In #2159, I moved tslint config into a standalone package and changed our packages to access that config directly, bypassing @loopback/build. It turns out this approach is breaking tslint extensions which require access to tslint via a peer dependency.

project -> @loopback/tslint-config -> tslint-extension (requires tslint as a peer dep)
project -> @loopback/build -> tslint (tslint is in a different part of the dep tree)

I see two ways how to fix this, see below. Considering the pros and cons, I am leaning towards Option A.

@raymondfeng @strongloop/loopback-maintainers Which option do you prefer? Can you think of any better solution?

Option A

Continue slimming down @loopback/build and move both tslint and typescript from dependencies into peer-dependencies. (tslint has a peer dep on typescript, that's why both packages has to be moved together.)

Pros: Users can decide what exact version of tsc & tslint they want to use and still keep using our builders. This can be very useful when tsc makes a breaking change in semver-minor release as it often does.

Also, users can decide what version of LoopBack's tslint config they want to use, independently on @loopback/build version.

Cons: We will have to duplicate the specification of typescript and tslint dev-dependencies in many places. At minimum in all example apps plus monorepo's root.

Option B

Keep the tslint config bundled inside @loopback/build. I think it still makes sense to have @loopback/tslint-config package (no changes there), but we need to revert the part of #2159 where we changed individual tslint config files (and the project template) to use @loopback/tslint-config directly.

Pros: No duplicated dev-dependencies, everything stays under @loopback/build umbrella.
Cons: Any breaking change in the tslint config will trigger a semver-major release of @loopback/build with all the implications for long term support (see Making breaking changes). Because most tslint config changes are backwards incompatible, we will be releasing semver-major versions of @loopback/build often.

Also, users of @loopback/build cannot decide which version of our tslint rules they want to use, they have to always use the version bundled inside the particular version of @loopback/build they depend on. (Same goes for tslint and tsc.)

bajtos · 2019-01-03T13:12:10Z

Upon further investigation of Option A, it turns out that removing build's dependency on typescript and tslint is too complicated as it breaks other things (e.g. integration tests). I think it's enough to change our project templates to add typescript and tslint as explicit dev-dependencies, and fix build to prefer project-local version of typescript/tslint over the version installed as build's dependency - see #2211

bajtos · 2019-01-04T08:26:07Z

I think it's enough to change our project templates to add typescript and tslint as explicit dev-dependencies,

Done in a5e1aef.

This pull request is ready for final review and landing as far as I am concerned.

@jannyHou @raymondfeng @strongloop/loopback-maintainers please review.

bajtos · 2019-01-04T15:22:38Z

Travis CI is failing in code-lint step, I'll investigate the problem next week.

> node packages/build/bin/run-tslint --project tsconfig.json
Failed to load /home/travis/build/strongloop/loopback-next/tslint.build.json: 
  Could not find custom rule directory: tslint-consistent-codestyle

jannyHou

Great effort! LGTM, just one nitpick suggestion for adding some doc.

jannyHou · 2019-01-07T04:28:43Z

bin/sync-dev-deps.js

+// License text available at https://opensource.org/licenses/MIT
+
+/**
+ * This is an internal script to synchronize versions of dev-dependencies


@bajtos When should we use this command? My guess is when updating a tslint/typescript version in the root level package.json?
A nitpick: maybe add some explanation in the documentation to remind developers to run it when updating dependencies.

packages/metadata/src/types.ts

bajtos · 2019-01-07T08:00:23Z

just one nitpick suggestion for adding some doc.
When should we use this command? My guess is when updating a tslint/typescript version in the root level package.json?
A nitpick: maybe add some explanation in the documentation to remind developers to run it when updating dependencies.

Good catch, I was thinking about the same too 👍

bajtos · 2019-01-07T08:23:27Z

Documentation added in 37f288a. Build fixed in 2e8c69f.

@jannyHou LGTY?

jannyHou

Cool 🚢

This change is necessary to allow `@loopback/tslint-config` to include tslint extensions from 3rd-party packages, because these extensions typically have a peer dependency on `tslint`. Another benefit is that projects can decide now which version of `tsc` and `tslint` they want to use, independently of the version bundled inside `@loopback/build`. To make it easier to keep typescript & tslint version specifiers in different places in sync, a new helper script is added to propagate changes in dev-dependencies from monorepo's root package.json to example projects: "npm run sync-dev-deps"

Fix code-lint to install dependencies of `@loopback/eslint-config`.

The built-in rule "no-unused-variable" is implemented in a hacky way that breaks other rules depending on TSC API. The 3rd-party rule "no-unused" does not require TSC API and thus can be checked from VS Code too. BREAKING CHANGE: The rule "no-unused-variable" has been replaced with "no-unused" rule. Code comments disabling "no-unused-variable" rule will no longer work and must be changed to disable "no-unused" rule instead.

b-admike

Nice work! 👏

bajtos self-assigned this Dec 14, 2018

bajtos requested review from raymondfeng, jannyHou and a team December 14, 2018 09:21

bajtos commented Dec 14, 2018

View reviewed changes

packages/metadata/src/types.ts Show resolved Hide resolved

bajtos mentioned this pull request Dec 14, 2018

Extract tslint config into a standalone package #2159

Merged

5 tasks

b-admike reviewed Dec 14, 2018

View reviewed changes

.vscode/tasks.json Outdated Show resolved Hide resolved

bajtos force-pushed the tslint/better-rule-for-unused-vars branch from edd29fb to 3676614 Compare December 14, 2018 17:04

jannyHou approved these changes Dec 14, 2018

View reviewed changes

packages/metadata/src/types.ts Show resolved Hide resolved

bajtos force-pushed the tslint/better-rule-for-unused-vars branch from 3676614 to 3669714 Compare January 3, 2019 09:51

bajtos mentioned this pull request Jan 3, 2019

fix(build): fix path lookup in build helpers #2211

Merged

1 task

bajtos force-pushed the tslint/better-rule-for-unused-vars branch from 3669714 to 37262cd Compare January 4, 2019 08:17

bajtos requested review from hacksparrow and marioestradarosa as code owners January 4, 2019 08:17

bajtos force-pushed the tslint/better-rule-for-unused-vars branch from 37262cd to 5ca18e0 Compare January 4, 2019 08:25

bajtos requested review from jannyHou and b-admike January 4, 2019 08:26

jannyHou approved these changes Jan 7, 2019

View reviewed changes

bajtos force-pushed the tslint/better-rule-for-unused-vars branch from 5ca18e0 to 2e8c69f Compare January 7, 2019 08:10

jannyHou approved these changes Jan 7, 2019

View reviewed changes

bajtos added 4 commits January 7, 2019 16:26

build: fix code-lint task for Travis CI builds

5fa042e

Fix code-lint to install dependencies of `@loopback/eslint-config`.

fix: rework tslint comments disabling "no-unused-variable" rule

6a400f2

bajtos force-pushed the tslint/better-rule-for-unused-vars branch from 2e8c69f to 6a400f2 Compare January 7, 2019 15:27

bajtos added developer-experience Issues affecting ease of use and overall experience of LB users refactor labels Jan 7, 2019

b-admike approved these changes Jan 7, 2019

View reviewed changes

bajtos merged commit a18a3d7 into master Jan 7, 2019

bajtos deleted the tslint/better-rule-for-unused-vars branch January 7, 2019 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tslint: use a better implementation of rule checking unused variables #2156

tslint: use a better implementation of rule checking unused variables #2156

bajtos commented Dec 14, 2018 •

edited

bajtos Dec 14, 2018

bajtos Dec 14, 2018

bajtos Dec 14, 2018

b-admike Dec 14, 2018 •

edited

bajtos Dec 14, 2018

b-admike Dec 14, 2018

bajtos Dec 14, 2018

bajtos Dec 14, 2018

bajtos commented Dec 14, 2018 •

edited

bajtos commented Dec 14, 2018

b-admike commented Dec 14, 2018

bajtos commented Dec 14, 2018

bajtos commented Jan 3, 2019

bajtos commented Jan 3, 2019

bajtos commented Jan 4, 2019

bajtos commented Jan 4, 2019

jannyHou left a comment

jannyHou Jan 7, 2019

bajtos commented Jan 7, 2019

bajtos commented Jan 7, 2019

jannyHou left a comment

b-admike left a comment

tslint: use a better implementation of rule checking unused variables #2156

tslint: use a better implementation of rule checking unused variables #2156

Conversation

bajtos commented Dec 14, 2018 • edited

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-admike Dec 14, 2018 • edited

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 Dec 14, 2018 • edited

bajtos commented Dec 14, 2018

b-admike commented Dec 14, 2018

bajtos commented Dec 14, 2018

bajtos commented Jan 3, 2019

bajtos commented Jan 3, 2019

bajtos commented Jan 4, 2019

bajtos commented Jan 4, 2019

jannyHou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Jan 7, 2019

bajtos commented Jan 7, 2019

jannyHou left a comment

Choose a reason for hiding this comment

b-admike left a comment

Choose a reason for hiding this comment

bajtos commented Dec 14, 2018 •

edited

b-admike Dec 14, 2018 •

edited

bajtos commented Dec 14, 2018 •

edited