Angular-cli: Remove no-use-before-declare and no-duplicate-variable from tslint.json because it is covered by no-var-keyword

[Martin-Wegner]

Bug Report or Feature Request (mark with an x)

- [x] bug report
- [ ] feature request

Versions.

@angular/cli: 1.0.0
node: 7.8.0
os: linux x64
@angular/common: 4.0.1
@angular/compiler: 4.0.1
@angular/core: 4.0.1
@angular/forms: 4.0.1
@angular/http: 4.0.1
@angular/platform-browser: 4.0.1
@angular/platform-browser-dynamic: 4.0.1
@angular/router: 4.0.1
@angular/cli: 1.0.0
@angular/compiler-cli: 4.0.1

### Repro steps.
-

### The log given by the failure.
-

### Desired functionality.
-

Mention any other details that might be useful.

The rule describtion (https://palantir.github.io/tslint/rules/no-use-before-declare/) for the no-use-before-declare rule says:

This rule is primarily useful when using the var keyword - the compiler will detect if a let and const variable is used before it is declared.

The rule describtion (https://palantir.github.io/tslint/rules/no-duplicate-variable/) for the no-duplicate-variable rule says:

This rule is only useful when using the var keyword - the compiler will detect redeclarations of let and const variables.

The Angular CLI created tslint.json contains the no-use-before-declare rule, the no-duplicate-variable and the no-var-keyword rule.
When using the no-var-keyword rule the no-use-before-declare rule and no-duplicate-variable rules can be dropped.

[dave11mj]

I fail to see why no-var-keyword would account for no-use-before-declare .. the description does say "primarily" when using var .. but I assume from the title no-use-before-declare also accounts for scenarios where I try to use a variable (let, const) without declaring it before hand.

[Martin-Wegner]

@dave11mj the no-var-keyword rule forces you to use only let or const. When using let or const the TypeScirpt compiler detects when a variable is used before it was declared or when a variable is declared twice in the same block scope => compile error. So the no-use-before-declare rule and no-duplicate-variable are unnecessary when using the no-var-keyword rule.

[dave11mj]

Hey @Martin-Wegner after reading your explanation, and playing around with the tslint rules, both of these only trigger when using the keyword var, but as mentioned since no var are allowed these rules become redundant.

[dave11mj]

Hey @Martin-Wegner and @hansl !

I did some additional testing to validate or invalidate this issue, and it seems the no-use-before-declare does have a purpose since it validates when one uses a const or let as I original thought. Below are the steps I took to validate or invalidate this issue.

_Note: For both tests temporarily set no-var-keyword to false_

For no-duplicate-variable

on app.component.ts add

// duplicated const
const a = 1;
const a = 2;

// duplicated let
let b = 1;
let b = 2;
b = 3;

// duplicated var
var c = 1;
var c = 2;

npm run lint only outputs app.component.ts[13, 5]: Duplicate variable: 'c', so it seems as mentioned by @Martin-Wegner and in the tslint docs for no-duplicate-variable

This rule is only useful when using the var keyword

since the cli has no-var-keyword set to true this rule can be considered redundant.

For no-use-before-declare

on app.component.ts add

export class AppComponent {
  title = 'app works!';
  testA = a;
  testB = b;
  testC = c;
}

// const used before declared
const a = 1;

// let used before declared
let b = 1;
b = 2;

// var used before declared
var c = 1;

npm run lint outputs

app.component.ts[10, 11]: variable 'a' used before declaration
app.component.ts[11, 11]: variable 'b' used before declaration
app.component.ts[12, 11]: variable 'c' used before declaration

Meaning with even though the tslint docs for no-use-before-declare say

This rule is primarily useful when using the var keyword

it does have purpose const and let even when no-var-keyword is set to false and should not be considered redundant.

Let me know if you guys have any others thoughts on this issue ! :D

[Martin-Wegner]

@dave11mj my point was that the TypeScript compiler complains about the code above. The task of a linter is not to find compilation errors.

[dave11mj]

@Martin-Wegner The code for no-use-before-declare doesn't cause any issues for me when I npm run build or ng serve .. The only thing warning stopping me from declaring all variables at the bottom of the file is the linter.

So unless you are advocating for declaring variables at the bottom of the file, no-use-before-declare seems to be a fair rule to keep.

[Martin-Wegner]

@dave11mj

You declared the variables outside of the class which means a kind of global variables. This seems to be OK for the TypeScript compiler...

Thanks for clarifying this.

[dave11mj]

No problem ! It was a bit tough to test at first since I was trying it out similarly to your screenshot. xD

End of the day no-duplicate-variable seems to be the only redundant one. ^^

[Martin-Wegner]

@dave11mj so much time for such a small change :)

[dave11mj]

I'd call it a step on the right direction towards tslint.json most users can agree on xD

[Martin-Wegner]

Since tslint 5:

WARNING: 'no-unused-variable' lint rule does not need to be set if the 'no-unused-locals' and 'no-unused-parameters' compiler options are enabled.

[Da13Harris]

I'm not sure I understand why no-duplicate-variable was removed instead of no-use-before-declare.

In my case, I'm trying to use the forwardRef() function from @angular/core to intentionally declare a token in advance.

Maybe there's a way to do this without breaking the no-use-before-declare rule?

[angular-automatic-lock-bot[bot]]

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._}