Base rule extension: no-var configuration for declarations

DanKaplanSES commented 6 months ago

Before You File a Proposal Please Confirm You Have Done The Following...

[X] I have searched for related issues and found none that match my proposal.
[X] I have searched the current extension rule list and found no rules that match my proposal.
[X] I have read the FAQ and my problem is not listed.

My proposal is suitable for this project

[X] My proposal specifically checks TypeScript syntax, or it proposes a check that requires type information to be accurate.
[X] I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Link to the base rule

https://eslint.org/docs/latest/rules/no-var

Description

Typescript global declarations are more subtle than I ever expected. For a better understanding, I recommend these SO answers:

Here are some key takeaways. If you want to add properties to globalThis:

When the typescript file has no import or export keyword (i.e., it's a script, not a module), it is done like so: declare var age: number.
When the typescript file does have import or export, you write this instead:
```
// an import / export line

declare global {
    var age: number;
}
```

If const or let is used instead of var, globalThis.age = 18 would error in both scenarios. So if the intention is to declare globalThis.age, it is necessary to use the var keyword: using const or let would be a mistake. When enabled, the base no-var rule indiscriminately marks all var usage as wrong, but in this case, it is required.

Fail

/*eslint no-var: "error"*/
export {}

var x = "y";
var CONFIG = {};

Pass

export {}

declare global {
    var age: number;
}

Additional Info

No response

Josh-Cena commented 6 months ago

Maybe this should be merged with #2594 and we should build an "extension rule" of no-Var called var that requires var usage in global declarations, but forbids var in other places?

bradzacher commented 6 months ago

Honestly I think we're just fine not doing anything here - declaration files are a fraction of a fraction of a percent of almost every codebase - so building and maintaining new lint rules with exceptions is really not worth the effort or burden, IMO.

Better to document the small edge-case as good usecase for a disable comment and move on.

The big reason I think a disable is better is because it serves as clear documentation for future readers. Not everyone will know that a var must be used in that location - so commenting like below serves as an educational tombstone to prevent someone accidentally breaking it in future.

// eslint-diaable-next-line no-var -- we must use a var here so that the global declaration works as expected. Using let/const would cause incorrect behaviour.

Josh-Cena commented 6 months ago

are a fraction of a fraction of a percent of almost every codebase

The fraction is low, but their existence is inevitable, though. Untyped dependencies, front end globals injected by scripts (like the Google API and FB API)...

DanKaplanSES commented 6 months ago

See also: #7613, #2594

Huh. It's really strange that this didn't show up when I searched. Thanks for letting me know.

Maybe this should be merged with https://github.com/typescript-eslint/typescript-eslint/issues/2594

I'm genuinely not sure if those failure cases are correct. Using let and const in a global type "works," it just works differently than a var. But is it bad to use let and const there?

@bradzacher

Better to document the small edge-case as good usecase for a disable comment and move on.

Do you mean eslint documentation will say this, or eslint users will write this documentation it in their projects?

The big reason I think a disable is better...

Sorry, do you mean, "better than implementing this issue," "better than disabling the rule," "better than leaving it as a warning/error," or something else entirely?

bradzacher commented 6 months ago

Sorry, do you mean ... something else entirely

I mean a disable comment, as I illustrated in the code block in my response.

Do you mean eslint documentation will say this

Unlikely to get such a comment into ESLint core's docs - but likely fits as an FAQ article or even just these issues serve as documentation as they contain the relevant context.

@Josh-Cena for sure - they exist, though the vast majority of .d.ts files aren't dealing with global declarations. Again the vast, vast, vast majority of dependencies these days have types, and those that don't are modules that don't influence globals.

So we're talking case that impacts a fraction of a fraction of the codebase (just .d.ts files) for a fraction of a fraction of dependencies (just untyped dependencies that declare global types).

Most TS devs will never run into this and those that do will likely only touch such a file once and never again.

For sure it's a completely valid concern and something that we probably should handle! But given we're already bandwidth constrained - it seems like something much better suited to "document and move on" rather than "implement an entire rule to handle"

DanKaplanSES commented 6 months ago

Thanks, @bradzacher I agree about the small number of eslint users this will affect.

I'd like to document where this comes up for me:

A DefinitelyTyped declaration file is out of sync with its implementation, and I have to troubleshoot the issue.
I'm porting JavaScript to TypeScript.
I'm trying to write a browser library that can't use modules for whatever reason.
I'm trying to write an extension for a library that was written before modules or I'm trying to contribute to its code base.
I'm using JSDom and it's not pragmatic to isolate the window global from the node global environment.

it seems like something much better suited to "document and move on" rather than "implement an entire rule to handle"

I empathize and that works fine for me. It's others I worry about. They have to learn what I've learned before they can "document and move on," and it took me a while to get to the point where I understood my symptoms. But yeah, I get it: there are much better bang for your buck issues out there. :)

PS: This is besides the point, but this proposed rule applies to all typescript files, not just declaration files. You can put declare global { ... } in both. I prefer putting it in .ts files so the declaration and the implementation are nearby.

JoshuaKGoldberg commented 6 months ago

@bradzacher @Josh-Cena did you two come to any conclusions here?

Personally I'd lean towards having two separate rules: a @typescript-eslint/no-var extension (this issue) and separately the nuanced rule around global declarations (#2594). They're pretty separate areas of concern and the latter can get nuanced.

bradzacher commented 6 months ago

This was my personal opinion

Most TS devs will never run into this and those that do will likely only touch such a file once and never again.

For sure it's a completely valid concern and something that we probably should handle! But given we're already bandwidth constrained - it seems like something much better suited to "document and move on" rather than "implement an entire rule to handle"

And I have the same opinion for #2594 - it's 3 years old and has zero engagement (no reactions or comments). It's not an issue the community cares for so we should close it.

Josh-Cena commented 5 months ago

I think the rule is simple enough and has good educational impacts, so I'm not against implementing it. People would turn it on indirectly through their preset anyway—they don't have to be aware of its existence.

bradzacher commented 5 months ago

and has good educational impacts

It wouldn't have any educational impact though - it would just stop erroring in this specific case. It wouldn't teach anyone anything!

Josh-Cena commented 5 months ago

The existence of this rule teaches people that they can and should use var in global definition files. We can add docs that further explain this. But I agree that the actual education part has to come from the other rule proposed about enforcing global var instead of const.

bradzacher commented 5 months ago

We already have the base rule turned on in our eslint-recommended config. If we add an extension rule we wouldn't create new reports here - we'd be removing reports.

So people don't know they have the rule turned on in the first place because it's turned on in a black-box extended config. And 99.999% of the time they don't get reports from the rule because people don't use var in modern TS. And in the rare case that someone does global augmentation they now don't get a report either.

So how are we improving education here? It seems to me like nobody would ever see a report or know the rule exists so nothing would change.

Josh-Cena commented 5 months ago

My point about "education" is not an error or a lack of error, which is why I agree a rule that enforces var is more educational because it gives you an error. My point is that:

The current state of erroring is counter-educational because people choose more awkward patterns that don't describe the runtime behavior faithfully, just to please the linter
The education comes from a documentation page. People would read it would understand why they need to relearn about var semantics in TS.

oosawy commented 2 weeks ago

I'd like to provide some context regarding how other toolchains are addressing this issue.

Biome implemented this feature earlier this year, while Deno lint, which supports TypeScript as a first-class citizen, doesn't seem to have implemented it yet.

I can't estimate the actual cost of adding and maintaining a new rule, but seeing by the implementation of the no-var rule, it doesn't seem like it would be overly long or complex 10-20 lines even with this feature.

From an educational perspective, it would be beneficial to have a rule like prefer-declare-global-with-var as well. Many users inadvertently define global variables by using the interface Window {} merging. At least eslint's no-var rule, which implies that using var is incorrect, steers developers in the wrong way.

In fact, there's an instance where I recommended using var in a review, only for it to be changed to interface Window {} not sure if this rule's error triggered it or not.

kirkwaiblinger commented 2 weeks ago

I'm strong +1 on requiring var in top level declarations.

The base rule report logic is 100% trivial (report on every VariableDeclaration[node.kind=var]), but has some nontrivial logic around the fixer, for variables in tdz or referenced outside the block in which their declaration occurs (which all matters much more for raw JS since the TS compiler will help you with use-before-declare errors and such).

All options for implementing this are easy IMO. Would take no more than a few hours.

If we wanted to create a separate rule for top-level declarations, we'd just have TSModuleDeclaration[kind=global] > TSModuleBlock > VariableDeclaration[kind!=var] and then, possibly something like Program > VariableDeclaration[declare=true][kind!=var] for globals in .d.ts files. Could include unconditional fixer since there's no runtime complications the way there are for no-var, though, tbh this rule might be better without a fixer, or at most suggestion.

If we wanted to create our own fork/extension of no-var, just query all VariableDeclarations and use the above conditions for when to require var, otherwise flag if var is used. Not sure if we'd even want to bring the fixer logic over if we fork the rule, but if we monkey patch it, it would be easy enough.

kirkwaiblinger commented 2 weeks ago

@DanKaplanSES to be clear, there is currently no @typescript-eslint/no-var rule. There is, externally, eslint's no-var rule, which is designed to work on JS. The question being considered in this issue is whether there are edge cases that occur when running that rule on TS code that are sufficiently misleading to warrant the creation of a @typescript-eslint/no-var rule to define TS-specific behavior. We'd refer to that as an "extension rule" or "base rule extension". When I said "fork" vs "extension", I was (somewhat confusingly) referring to the implementation details of the rule's code, which wouldn't have any user-facing impact (including on docs or anything else). The proposed extension would show up to the user just like, for example, require-await (https://typescript-eslint.io/rules/require-await/)

DanKaplanSES commented 2 weeks ago

@DanKaplanSES to be clear, there is currently no @typescript-eslint/no-var rule. There is, externally, eslint's no-var rule, which is designed to work on JS.

Ah, right. I had forgotten that since I first created this issue. Thanks for the reminder.

The question being considered in this issue is whether there are edge cases that occur when running that rule on TS code that are sufficiently misleading to warrant the creation of a @typescript-eslint/no-var rule to define TS-specific behavior. We'd refer to that as an "extension rule" or "base rule extension".

Ah, okay. In that case, I'm preaching to the choir in my previous reply. I'm going to delete it since I think it would waste everyone's time to read it.

Thanks for clarifying!

typescript-eslint / typescript-eslint