See also: #7613, #2594

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?

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.

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

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 #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?

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"

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:

1. A DefinitelyTyped declaration file is out of sync with its implementation, and I have to troubleshoot the issue.
2. I'm porting JavaScript to TypeScript.
3. I'm trying to write a browser library that can't use modules for whatever reason.
4. 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.
5. 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 (EDIT: I documented this journey here). 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.

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

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.

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.

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!

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.

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.

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:

1. 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
2. The education comes from a documentation page. People would read it would understand why they need to relearn about var semantics in TS.

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.

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.

Our students were also running problems after we enabled the no-namespace rule, with both no-var reports on declare global...var and no-namespace reports on declare module globalThis...let:

• Docs: globalThis without ignores of no-var and no-namespace #9582

We discourage disabling lint rules as they are learning.

I proposed in the issue above that a blessed syntax be documented, but maybe it's better to instead change the no-var reporting - gut feeling is good on this option... 🤔

Coming over from #9865, I think I've landed in the camp of making a @typescript-eslint/var extension rule as @Josh-Cena and @kirkwaiblinger have suggested. The particularly convincing points for me were that:

• declare global contexts do pop up once in a while
• Having a dedicated docs page would be helpful for showing how to use a declare global var properly

#7941 (comment) in particular too.

My 2C:
In almost 12 months this issue has garnered just 4 community reactions.
It just doesn't seem like this is something that's important to the broader community and thus isn't worth the implementation or maintenance cost.

It's not a bad practice developers are trying to lint out.
It's not widespread enough that adding a few eslint-disables is a nuisance.
It's even subtle enough that many just declare global consts and move on even when that doesn't reflect runtime reality.
All these mean we don't get strong incentive to vote on this, but that doesn't mean the rule is not good. Rather, exactly because of these points, it provides excellent educational opportunities if we can enforce the inverse and either use documentation or an explicit error to tell people to prefer var declarations.

but that doesn't mean the rule is not good

To be clear - I didn't say it wasn't.
I said that I don't think that there is perceived community value in this. If people don't care or want it then why spend time building and maintaining a rule?

Personally I'd sooner see us remove no-var from eslint-recommended than I would see us build an extension rule just to handle this edge case.

I think that we also need to consider our own biases here.

We're all biased because we are all power users. We have setup many codebases and maintained infrastructure. We have run into this and so it's something we're aware of.

By-and-large the average TS user doesn't touch global declarations and the like. They don't run into this issue because they don't encounter the problem.

Put it another way. Is it worth burning our very limited bandwidth building and maintaining an extension rule when it only impacts "users that are writing global declarations in situations where they need the global to be writeable".

It's a very specific usecase. AIUI the immutable global case doesn't matter cos you can use a const, right? It's just the mutable case. Which, I might add, is broadly considered a very smelly anti-pattern by today's standard.

We all have -1'd more common usecases than that. I don't think that we should be continuing with this just because we're personally invested.
By all processes and definitions we have written out and followed elsewhere this issue fits the bill of a "close and move on".

By-and-large the average TS user doesn't touch global declarations and the like.

Is this true? I would have thought that the global mutable var pattern would not be so niche - it's quite useful for simple global singletons, not sure I would classify this as a code smell or an antipattern.

With the advent of ESM -- yeah it is definitely niche and smelly!

Why would you create a global singleton via a global var when you can neatly export the singleton from a module and import it where required?
Augmenting global means:

• code analysis is hard -- you now have an implicit, untraceable dependency between modules.
  • EG bundlers cannot track where the global was defined and thus you could create a bundle without the required declaration spot -- creating a broken bundle.
• malicious actors can interact with your internals because it's readily available on global/window

Both of these are non-issues with ESM:

class MySingleton { ... }
export { type MySingleton };

const instance = new MySingleton();
export default instance;

// or perhaps you want a lazy init pattern
let instance: MySingleton | null = null;
export default function getOrInit(): MySingleton {
  if (instance == null) {
    instance = new MySingleton();
  }
  return instance;
}

• You can use tsconfig paths / bundler resolution config to create a path-less import if you want it to be simple like import singleton from 'MySingleton';
• This is immediately analysable by bundlers and tree-shakers alike.
• This singleton is not accessible outside your application closure.

Hmm, you're giving me something to consider 🤔 - maybe using an IIFE closure would be fine for keeping a single database connection across hot reloading (at least for the first case that I'm thinking of now... not to say that I have thought of every possible use case of global mutable state):

Before (with globalThis)

declare module globalThis {
  let postgresSqlClient: Sql;
}

// Connect only once to the database
// https://github.com/vercel/next.js/issues/7811#issuecomment-715259370
function connectOneTimeToDatabase() {
  if (!('postgresSqlClient' in globalThis)) {
    globalThis.postgresSqlClient = postgres(postgresConfig);
  }

  // Workaround to force Next.js Dynamic Rendering on every database query
  // https://github.com/vercel/next.js/discussions/50695
  return ((
    ...sqlParameters: Parameters<typeof globalThis.postgresSqlClient>
  ) => {
    noStore();
    return globalThis.postgresSqlClient(...sqlParameters);
  }) as typeof globalThis.postgresSqlClient;
}

export const sql = connectOneTimeToDatabase();

Source: https://github.com/upleveled/next-js-example-spring-2024-atvie/blob/34d5027a5670852b11c8812e0ac3c99cb0d933a4/database/connect.ts#L9-L41

After (with local let variable in IIFE closure)

// Connect only once to the database
// https://github.com/vercel/next.js/issues/7811#issuecomment-715259370
const connectOneTimeToDatabase = (() => {
  let postgresSqlClient: Sql;

  return () => {
    if (typeof postgresSqlClient === 'undefined') {
      postgresSqlClient = postgres(postgresConfig);
    }

    // Workaround to force Next.js Dynamic Rendering on every database query
    // https://github.com/vercel/next.js/discussions/50695
    return ((
      ...sqlParameters: Parameters<typeof postgresSqlClient>
    ) => {
      noStore();
      return postgresSqlClient(...sqlParameters);
    }) as typeof postgresSqlClient;
  };
})();

export const sql = connectOneTimeToDatabase();

At the very least if you're going to use a global mutable variable for your DB client you should use a non-enumerable property with a module-private Symbol key for security so that someone can't grab your db client and mutate your db directly! Or worse - swap it out for their own client and steal your data!

I guess drifting a bit off topic here (feel free to minimize this comment), but by "someone" I'm assuming you mean some code running in the Node.js global execution context? I guess if you have untrusted code running there you have bigger problems anyway and a symbol won't protect much.

Symbol properties do not grant you extra safety, because of getOwnPropertySymbols.

Wait what? Who specced those functions out to enumerate non-enumerable keys?!?
Thanks I hate it.

But yes we are way off topic now!

Hmm, you're giving me something to consider 🤔 - maybe using an IIFE closure would be fine for keeping a single database connection across hot reloading (at least for the first case that I'm thinking of now... not to say that I have thought of every possible use case of global mutable state)

So we looked into the IIFE + closure option above, and it looks like we still need globalThis:

1. React Fast Refresh (aka "hot reloading" or "live reloading") uses webpack Hot Module Replacement (React) or Metro Hot Module Replacement (React Native 0.61+) (history)
2. webpack and Metro HMR both re-initialize the top-level module scope when changes are made in a tree
3. This leads to the connectOneTimeToDatabase constant being re-initialized, discarding the old closure and database connection / client (but not ending the connection, leading to an accumulation of connections, eventually leading to the PostgreSQL error remaining connection slots are reserved for non-replication superuser connections)
4. There appears to be module.hot.dispose(<cleanup fn>) or import.meta.webpackHot.dispose(<cleanup fn>), which would appear to be an option to end the database connection on every hot refresh, but callbacks passed to this are not called (potentially because of the automatic wrapping of React Fast Refresh)
5. Also, even if the hot.dispose option worked, it would be inefficient to always create and destroy new database connections on every hot reload

So it's my conclusion from all of this that globalThis is indeed the correct pattern for hot module replacement in dev server scenarios - not niche, a code smell or an antipattern.

Others also confirm this approach:

1. Dev mode hot reload loses database connection · vercel/next.js · Discussion #26427
2. How to preserve a database connection pool in hooks.server.js during hot reload in SvelteKit with Vite.js and mariadb? - Stack Overflow
3. etc.

This is of course only one use case for globalThis - gut feel says there are probably more non-smelly, non-antipattern, valid uses of it.

I dunno but to me it sounds like the global variable is easier, but is also hiding a bug in your dev env that could lead to unsafety or undefined behaviour.

You're spawning a db connection using one set of code. After a hot reload you have a new set of code that could be expecting to use a db connection that's setup with a different config.

So the only way to fix this state would be to hard reload your dev env to forcefully drop the connection and then recreate the entire connection using the new code.

This bug would only occur with changes to the database configuration:

• In dev environment
• Making a change which will trigger hot reloading
• Making a change to the database configuration

So, I would argue, more of an edge case. If this is a common pain point for developers (which I would doubt), then a comment at the database configuration code "Restart your dev server on any changes" would resolve it. Or if it's a common pain point and you want to allow developers to ignore comments: a wrapper script for the dev server to restart it on changes to the database config.

The main point here for this issue is that IMO this is a valid usage of mutable globalThis.

To be clear: I never said it wasn't valid to use globalThis.
I specifically said:

By-and-large the average TS user doesn't touch global declarations and the like.

Which I still hold to be true.

In modern code generally it's a code smell to rely on mutable global variables and the vast, vast, vast majority of usecases, and in general people are instead going to rely solely on ESM to encapsulate things and declare dependencies explicitly.

That doesn't mean there aren't valid usecases for mutable variables in globalThis -- it's just that generally it is considered an anti-pattern or a code smell to use it in modern web because the alternatives are generally better.

By-and-large the average TS user doesn't touch global declarations and the like.

Exactly, global declarations is not everyday things, but there are 2.5k declare global { var... } and at least 10k+ interface Window {}. There may be some interpretations to the numbers, my concerns are the most of interface Window {} could be mistake as you know and caused by lack of knowledge or the no-var rule.
So it is not the linter's place to lead from a syntax that is well used and has valid use cases to a syntax that looks right but is wrong.

When I've worked with and taught other developers, there are sometimes points where they need to use a global variable. When they find declare global through searching and then see linter errors, what ends up happening is they assume they're on the wrong path. The end result is always to switch to any ((window as any).globalFoo) since they know it works.

I'd prefer the linter to default to supporting declare global without the need to disable rules, as it helps lead developers in the right direction.

@NotWoods I think you bring up a separate issue. This one is about using declare var (within a declare global), not declare global.

I think this issue can be closed in favor of eslint/eslint#19714.

eslint/eslint#19714 is a PR, not an issue.

Hopefully it is a good solution to this issue however.