Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Martin-Luft opened this issue Mar 30, 2017 · 13 comments · Fixed by #5783
Assignees
Labels
effort1: easy (hours) P5 The team acknowledges the request but does not plan to address it, it remains open for discussion severity1: confusing

Comments

@Martin-Luft
Copy link

Martin-Luft commented Mar 30, 2017

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
Copy link
Contributor

dave11mj commented Mar 30, 2017

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-Luft
Copy link
Author

Martin-Luft commented Mar 31, 2017

@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.

@Martin-Luft Martin-Luft changed the title Remove no-use-before-declare from tslint.json because it is covered by no-var-keyword Remove no-use-before-declare and no-duplicate-variable from tslint.json because it is covered by no-var-keyword Mar 31, 2017
@dave11mj
Copy link
Contributor

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
Copy link
Contributor

dave11mj commented Apr 1, 2017

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-Luft
Copy link
Author

@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
Copy link
Contributor

dave11mj commented Apr 3, 2017

@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.

image

image

image

@Martin-Luft
Copy link
Author

@dave11mj

image

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
Copy link
Contributor

dave11mj commented Apr 3, 2017

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-Luft
Copy link
Author

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

@dave11mj
Copy link
Contributor

dave11mj commented Apr 3, 2017

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

@Martin-Luft
Copy link
Author

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.

@filipesilva filipesilva self-assigned this May 3, 2017
@filipesilva filipesilva added effort1: easy (hours) P5 The team acknowledges the request but does not plan to address it, it remains open for discussion severity1: confusing labels May 3, 2017
filipesilva pushed a commit to filipesilva/angular-cli that referenced this issue May 9, 2017
@Da13Harris
Copy link

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
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
effort1: easy (hours) P5 The team acknowledges the request but does not plan to address it, it remains open for discussion severity1: confusing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants