build: move unused-variable check from tslint to tsc #1648

bajtos · 2018-08-27T08:02:08Z

Now that we no longer need additional imports of referenced types to satisfy the compiler (see #1641), we have the opportunity to enable unused-variable checking at TypeScript level.

This pull request has two parts:

build: move unused-variable check from tslint to tsc (94c4440)

Disable tslint's "no-unused-variable" rule, it has been deprecated.

Enable TypeScript rule "noUnusedLocals", rework all places violating
this rule to use a different mechanism for turning this rule off.

Checking unused variables at compile time has the following benefits:

Unused variables (locals) are highlighted directly in VS Code.
Before, we had to run tslint to discover unused variables, since
the VS Code extension for tslint was not able to detect unused vars.
The output of tslint is clean again with no deprecation warnings
cluttering the output.

build: fix tslint issues related to promises (fd3317e)

After I disabled no-unused-variable check, tslint started to complain about places incorrectly using promises. Some of the warnings were legitimate and I fixed the code. Other cases were not valid, I fixed them by modifying tslint config and adding comments to disable the problematic rule for few source code lines.

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

Disable tslint's "no-unused-variable" rule, it has been deprecated. Enable TypeScript rule "noUnusedLocals", rework all places violating this rule to use a different mechanism for turning this rule off. Checking unused variables at compile time has the following benefits: - Unused variables (locals) are highlighted directly in VS Code. Before, we had to run tslint to discover unused variables, since the VS Code extension for tslint was not able to detect unused vars. - The output of tslint is clean again with no deprecation warnings cluttering the output.

jannyHou

LGTM 👍

raymondfeng · 2018-08-27T14:29:39Z

FYI: palantir/tslint#4100

raymondfeng · 2018-08-27T14:34:02Z

packages/context/test/unit/inject.unit.ts

    class TestClass {
-      constructor(@inject('foo') foo: string) {}
+      constructor(@inject('foo') public foo: string) {}


Is the following another way to silent the compiler?

constructor(@inject('foo') foo: string) { disableUnusedLocalError(foo); }

IMO, these two lines are functionally different:

constructor(@inject('foo') foo: string) {} // foo is a local arg constructor(@inject('foo') public foo: string) {} // foo becomes a public property of TestClass

IMO, these two lines are functionally different

Yes, those lines are different. I thought that difference does not matter in this particular unit test.

Is the following another way to silent the compiler?

It should be, I can give it a try if we agree this is the direction to pursue.

raymondfeng · 2018-08-27T14:35:01Z

packages/context/test/unit/inject.unit.ts

    }
+    disableUnusedLocalError(TestClass);


Sigh, this is ugly but it does not seem that TS has fine control or comment based rules.

raymondfeng · 2018-08-27T14:37:15Z

packages/testlab/src/disable-unused-local-error.ts

+ *
+ * @param value A value to mark as used for the compiler.
+ */
+export function disableUnusedLocalError<T>(value: T) {


Do we expect disableUnusedLocalError to be used in tests only? In most cases, @loopback/testlab is a dev dependency. What if we have some code in src that want to use this hack?

Do we expect disableUnusedLocalError to be used in tests only? In most cases, @loopback/testlab is a dev dependency. What if we have some code in src that want to use this hack?

Good point. I hope we won't have unused dependences in production code - that's why we are enabling this linting/compiler rule in the first place. So far, the production code does not need this hack after tsc fixed re-exports of implicit dependencies in [email protected].

Having said that, if there comes a time that we need to keep an unused local in our code, then we will indeed need something like disableUnusedLocalError to be available in production code too.

b-admike

build: fix tslint issues related to promises (fd3317e)

This commit LGTM, but for removing unused-variable check from tslint to tsc, we just need to decide based on the pros and cons of the switch and if the team agrees, I'm fine with it.

virkt25 · 2018-08-27T15:11:54Z

packages/build/config/tslint.build.json

@@ -15,9 +15,8 @@
    // User-land promises like Bluebird implement "PromiseLike" (not "Promise")
    // interface only. The string "PromiseLike" bellow is needed to
    // tell tslint that it's ok to `await` such promises.
-    "await-promise": [true, "PromiseLike"],
-    "no-floating-promises": true,
-    "no-unused-variable": true,


I had looked into making this PR but decided against it based on palantir/tslint#4100 ... TypeScript doesn't recommend using the noUnusedLocals flag as an alternative to linting which provides finer line-by-line control. Maybe we should keep this while also using the noUnusedLocals flag ... at least until there is a final decision from tslint on deprecating or undeprecating the rule.

bajtos · 2018-08-27T15:22:08Z

I had looked into making this PR but decided against it based on palantir/tslint#4100

To be honest, I am not entirely happy with disableUnusedLocalError hack.

Based on what I read in palantir/tslint#4100 and the linked discussions in tsc, I am no longer sure whether it's better to use tslint or tsc to enforce this rule.

My main objection against tslint is that the VSCode extension is not able to detect unused variables inside IDE and thus I usually discover them just when I think that I am done with coding and can git commit my changes. A small annoyance I can live with.

jannyHou · 2018-08-27T17:15:27Z

Given the concern post in #1648 (comment) I tend to not removeno-unused-variable from tslint.
BTW, I am a little confused by

Unused variables (locals) are highlighted directly in VS Code.
Before, we had to run tslint to discover unused variables, since
the VS Code extension for tslint was not able to detect unused vars.

the unused variable/constant are grey in my editor even without adding the rule in tsconfig:

Did I miss something? 🤔 And if we do need noUnusedLocals, we can add it and still keep no-unused-variable

virkt25 · 2018-08-28T02:28:05Z

A small annoyance I can live with.

It is a small annoyance indeed ... maybe it'll be resolved once tslint and tsc figure out their respective roles in enforcing this rule. To me it's the Deprecation warning that I'm finding annoying at this point but 🤷‍♂️

bajtos · 2018-08-28T12:20:21Z

Based on the discussion above and a poll we took in our SlackChat, I am closing this pull request as rejected and going to open a new PR with the second commit.

bajtos added 2 commits August 27, 2018 09:19

build: fix tslint issues related to promises

fd3317e

bajtos added this to the August Milestone milestone Aug 27, 2018

bajtos self-assigned this Aug 27, 2018

bajtos requested review from raymondfeng, virkt25, b-admike and hacksparrow August 27, 2018 08:02

jannyHou approved these changes Aug 27, 2018

View reviewed changes

raymondfeng reviewed Aug 27, 2018

View reviewed changes

b-admike reviewed Aug 27, 2018

View reviewed changes

virkt25 reviewed Aug 27, 2018

View reviewed changes

bajtos closed this Aug 28, 2018

bajtos deleted the no-unused-locals branch August 28, 2018 12:21

bajtos mentioned this pull request Aug 28, 2018

build: fix tslint issues related to promises #1651

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: move unused-variable check from tslint to tsc #1648

build: move unused-variable check from tslint to tsc #1648

bajtos commented Aug 27, 2018 •

edited

Loading

jannyHou left a comment

raymondfeng commented Aug 27, 2018

raymondfeng Aug 27, 2018

bajtos Aug 27, 2018

raymondfeng Aug 27, 2018

raymondfeng Aug 27, 2018

bajtos Aug 27, 2018

b-admike left a comment •

edited

Loading

virkt25 Aug 27, 2018

bajtos commented Aug 27, 2018

jannyHou commented Aug 27, 2018

virkt25 commented Aug 28, 2018

bajtos commented Aug 28, 2018

build: move unused-variable check from tslint to tsc #1648

build: move unused-variable check from tslint to tsc #1648

Conversation

bajtos commented Aug 27, 2018 • edited Loading

Checklist

jannyHou left a comment

Choose a reason for hiding this comment

raymondfeng commented Aug 27, 2018

raymondfeng Aug 27, 2018

Choose a reason for hiding this comment

bajtos Aug 27, 2018

Choose a reason for hiding this comment

raymondfeng Aug 27, 2018

Choose a reason for hiding this comment

raymondfeng Aug 27, 2018

Choose a reason for hiding this comment

bajtos Aug 27, 2018

Choose a reason for hiding this comment

b-admike left a comment • edited Loading

Choose a reason for hiding this comment

virkt25 Aug 27, 2018

Choose a reason for hiding this comment

bajtos commented Aug 27, 2018

jannyHou commented Aug 27, 2018

virkt25 commented Aug 28, 2018

bajtos commented Aug 28, 2018

bajtos commented Aug 27, 2018 •

edited

Loading

b-admike left a comment •

edited

Loading