Skip to content

feat: allow global type declaration in no-var #19714

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.

Sign up for GitHub

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

Merged
snitin315 merged 5 commits into from
Jun 6, 2025
Merged

feat: allow global type declaration in no-var #19714

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
abd7209
feat: allow global type declaration in `no-var`
remcohaszing May 8, 2025
abbdd88
chore: specify in meta no-var supports TypeScript
remcohaszing May 9, 2025
2678514
chore: use separate RuleTester for no-var TS support
remcohaszing May 9, 2025
366a4cf
docs: document TypeScript handling for no-var
remcohaszing May 12, 2025
b3ab2a9
chore: run trunk fmt
remcohaszing May 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions lib/rules/no-var.js
View file
Open in desktop
Copy link
Contributor

@kirkwaiblinger kirkwaiblinger May 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the rule logic - I was thinking this should also apply to top-level definitions in (non-module) declaration files, right? Been a while since I've worked with that particular setup. But something like this?

// foo.d.ts
declare var globalVar: string;

Copy link
Contributor

@kirkwaiblinger kirkwaiblinger May 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a little minimal repository demonstrating this behavior for reference: https://github.com/kirkwaiblinger/tsglobalvardemo

Copy link
Contributor

@kirkwaiblinger kirkwaiblinger May 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Come to think of it, it actually doesn't matter whether it's a .d.ts file or not. The same behavior applies with declared variables in ordinary .ts files.

// foo.ts

// setup
declare var globalVar: string;
declare let globalLet: string;
declare const globalConst: string;

// read unqualified
console.log(globalConst); // works
console.log(globalLet); // works
console.log(globalVar); // works

// write unqualified
globalConst = 'doesnt work';
globalLet = 'works';
globalVar = 'works';

// read on globalThis
console.log(globalThis.globalConst); // doesnt work
console.log(globalThis.globalLet); // doesnt work
console.log(globalThis.globalVar); // works

// write on globalThis
globalThis.globalConst = 'doesnt work';
globalThis.globalLet = 'doesnt work';
globalThis.globalVar = 'works';

// read on self
console.log(self.globalConst); // doesnt work
console.log(self.globalLet); // doesnt work
console.log(self.globalVar); // works

// write on self
self.globalConst = 'doesnt work';
self.globalLet = 'doesnt work';
self.globalVar = 'works';

Copy link
Contributor Author

@remcohaszing remcohaszing May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What matters is whether TypeScript considers file to be a script or a module. If a module contains imports or exports, it’s a module. Otherwise it’s a script. In a script everything you declare is global. In a module you can declare globals with declare global.

So this module:

declare global {
  var x: number;
}

export {}

is equivalent to this script:

declare var x: number;

However, TypeScript also has the option moduleDetection. If this is set to force, everything source file considered a module. This makes it ambiguous whether the latter is a local variable declaration or a global. We could assume default options. The moduleDetection option doesn’t apply to declaration files.

Original file line number Diff line number Diff line change
Expand Up @@ -346,9 +346,19 @@ module.exports = {

return {
"VariableDeclaration:exit"(node) {
if (node.kind === "var") {
report(node);
if (node.kind !== "var") {
return;
Copy link
Contributor

@kirkwaiblinger kirkwaiblinger May 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In typescript-eslint/typescript-eslint#7941, the proposal is to require var for global ambient declarations rather than just to permit it. Is the discrepancy in behavior here, where non-vars are simply ignored, intentional?

Copy link
Contributor Author

@remcohaszing remcohaszing May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s #2594, though there is discussion about it in #7941. I have been fiddling a bit to implement #2594. I don’t think it should have been closed as a duplicate. They’re two different things.

Copy link
Contributor

@kirkwaiblinger kirkwaiblinger May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I see what you're saying...

However, I think it's not such a good idea to have two separate rules, especially in two separate plugins, handle overlapping and possibly-conflicting logic. It's like if return-await were actually two separate rules for when return await is required and when it's prohibited, and those two rules were in separate plugins, and one of them didn't even have access to the TS API required for correctness in some cases.

What do you think of the following proposal instead? What if we draw the line such that this rule, no-var, ignores all TS ambient declarations, even those that shouldn't be vars, and then the typescript-eslint rule (maybe called something like declare-var) would have the job of properly enforcing or prohibiting vars in all cases for declared variables. This way detailed TS knowledge stays out of no-var and the dividing line between what's handled by typescript-eslint and what's handled by no-var is much more clear.

Concretely, that would mean I propose that all of the following, currently documented as incorrect, would be permitted by no-var...

declare var x: number

declare namespace ns {
	var x: number
}

declare module 'module' {
	var x: number
}

and instead would be banned by a new typescript-eslint rule.

Copy link
Contributor

@kirkwaiblinger kirkwaiblinger May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, I most prefer having all the logic handled in this no-var rule, including enforcing declare var where required, at the possible expense of a niche edge case as mentioned in #19714 (comment).

But if we're not going to enforce declare var where required in this rule, I think we should draw the line at no-var ignoring declare var completely.

Copy link
Member

@nzakas nzakas May 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the most correct implementation is as you suggested in #19714 (comment). no-var should not be requiring var anywhere, it should be allowing var where another option doesn't make sense.

Copy link
Contributor Author

@remcohaszing remcohaszing May 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this even applies to regular code, not just declarations. For example, the following file is also a global script as far as TypeScript is concerned. It doesn’t even matter if it’s JavaScript or TypeScript syntax.

var a = 1

How far should the no-var exceptions go? Also how many users will actually run into this? I think most code is written as a module today, or at least that’s often the intention.

There are only 2 cases where the user should definitely use var:

  1. Within declare global, which is handled by this PR.
  2. Within a TypeScript declaration file that doesn’t contain ESM syntax. This can be detected based on the file name (*.d.ts / *.d.cts / *.d.mts / *.d.*.ts). Should I add support for these?

Copy link
Member

@nzakas nzakas May 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original intent of this rule is to ban the use of var completely. The goal of this PR is to mark as an exception those places in TypeScript where changing from var to something else either isn't possible or changes the behavior in an undesirable way.

2. Within a TypeScript declaration file that doesn’t contain ESM syntax. This can be detected based on the file name (*.d.ts / *.d.cts / *.d.mts / *.d.*.ts). Should I add support for these?

At this point I'd say no. This feels like a step too far. A better approach for a user is to disable this rule just in those files.

}

if (
node.parent.type === "TSModuleBlock" &&
node.parent.parent.type === "TSModuleDeclaration" &&
node.parent.parent.global
) {
return;
}

report(node);
},
};
},
Expand Down
20 changes: 20 additions & 0 deletions tests/lib/rules/no-var.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,12 @@ ruleTester.run("no-var", rule, {
parserOptions: { ecmaFeatures: { globalReturn: true } },
},
},
{
code: "declare global { var bar: 'car' }",
languageOptions: {
parser: require("@typescript-eslint/parser"),
},
},
],

invalid: [
Expand Down Expand Up @@ -429,5 +435,19 @@ ruleTester.run("no-var", rule, {
{ messageId: "unexpectedVar" },
],
},
{
code: "declare namespace ns { var bar: 'car' }",
languageOptions: {
parser: require("@typescript-eslint/parser"),
},
errors: [{ messageId: "unexpectedVar" }],
},
{
code: "declare module 'module' { var bar: 'car' }",
languageOptions: {
parser: require("@typescript-eslint/parser"),
},
errors: [{ messageId: "unexpectedVar" }],
},
],
});
Loading