no-var-keyword: Allow global var declarations #2513
Conversation
src/language/utils.ts
Outdated
|
|
||
| return isNodeFlagSet(parentNode!, ts.NodeFlags.Let) | ||
| || isNodeFlagSet(parentNode!, ts.NodeFlags.Const); | ||
| export function isBlockScopedVariableDeclarationList(node: ts.VariableDeclarationList): boolean {// tslint:disable-next-line no-bitwise |
There was a problem hiding this comment.
this function is already available from tsutils, so there is no need to duplicate it here
src/rules/noVarKeywordRule.ts
Outdated
| function walk(ctx: Lint.WalkContext<void>): void { | ||
| const { sourceFile } = ctx; | ||
| return ts.forEachChild(ctx.sourceFile, function cb(node: ts.Node): void { | ||
| switch (node.kind) { |
There was a problem hiding this comment.
you could simplify the condition and replace the whole switch statement with this:
if (isVariableDeclarationList(node) && !isBlockScopedVariableDeclarationList(node) &&
(node.parent!.kind !== ts.SyntaxKind.VariableStatement || !isGlobalVarDeclaration(node.parent!)))
fail(node);you could even consider inlining fail
src/rules/noVarKeywordRule.ts
Outdated
| function fail(node: ts.Node): void { | ||
| // Don't apply fix in a declaration file, because may have meant 'const'. | ||
| const fix = sourceFile.isDeclarationFile ? undefined : new Lint.Replacement(node.getStart(), "var".length, "let"); | ||
| ctx.addFailureAtNode(Lint.childOfKind(node, ts.SyntaxKind.VarKeyword)!, Rule.FAILURE_STRING, fix); |
There was a problem hiding this comment.
there is no need for addFailureAtNode with childOfKind, you already computed the position of the var keyword for the fix one line above.
src/rules/noVarKeywordRule.ts
Outdated
| this.createReplacement(nodeStart, "var".length, "let")); | ||
| } | ||
| function isDeclareGlobal(node: ts.Node): boolean { | ||
| return isModuleDeclaration(node) && node.name.kind === ts.SyntaxKind.Identifier && node.name.text === "global"; |
There was a problem hiding this comment.
ModuleBlock.parent is always a ModuleDeclaration. typescript@next contains the correct types.
for now it is save to just assert the type
src/rules/noVarKeywordRule.ts
Outdated
| super.visitForOfStatement(node); | ||
| function fail(node: ts.Node): void { | ||
| // Don't apply fix in a declaration file, because may have meant 'const'. | ||
| const fix = sourceFile.isDeclarationFile ? undefined : new Lint.Replacement(node.getStart(), "var".length, "let"); |
There was a problem hiding this comment.
consider passing sourceFile as argument to node.getStart()
src/rules/noVarKeywordRule.ts
Outdated
| this.createReplacement(nodeStart, "var".length, "let")); | ||
| } | ||
| function isDeclareGlobal(node: ts.Node): boolean { | ||
| return isModuleDeclaration(node) && node.name.kind === ts.SyntaxKind.Identifier && node.name.text === "global"; |
There was a problem hiding this comment.
you could even simplify this further:
return (node.flags & ts.NodeFlags.GlobalAugmentation) !== 0;In addition you avoid false positived for namespace foo.global {} and namespace foo { export namespace global {} } (those are also good candidates for a test)
You can even consider inlining this condition.
ghost
deleted the
PR checklist
Overview of change:
The
varkeyword is useful in one case: When you want to declare a global variable.The problem with declaring a global
constis that this is considered a block-scoped declaration, and will warn for duplicates.This PR changes the rule to allow
varin a position where it declares a global variable.It still warns for:
export var x: number;(not global)var x;(not a declaration if not indeclare global)Note: Also disabled fixer in declaration files, because those should almost always be
constinstead oflet, and we can't detectprefer-constin declarations (#2390).CHANGELOG.md entry:
[enhancement]
no-var-keyword: Allow global var declarations