no-typeof-undefined should exclude globally declared variables like window

[ian-craig]

Bug Report

• TSLint version: 5.8.0
• TypeScript version: 2.7.0
• Running TSLint via: VSCode

TypeScript code being linted

declare var window: any;

if (typeof window !== "undefined") {
    // do something 
}

with tslint.json configuration:

{
    "rules": {
        "no-typeof-undefined": true,
    },
}

Actual behavior

TSLint gives error

[tslint] Avoid typeof x === 'undefined' comparisons. Prefer x == undefined or x === undefined: typeof window !== "undefined" (no-typeof-undefined)

The suggested fix would be:

if (window !== undefined) {
    // do something 
}

which results in ReferenceError: 'window' is undefined at...

Expected behavior

Ignore typeof foo !== "undefiend" when foo has been declared globally.

[estaub]

A linter for the OPPOSITE of this rule would be useful for globals!

This is a common test in code that may run in a browser or under node.js, like server-side-rendered code.

[HamletDRC]

Does any object other than 'window' display this error?

[HamletDRC]

i.e. why should we change this rule for all globals instead of just window?

[estaub]

@HamletDRC I'm misunderstanding, I suspect. AFAICT, this has nothing to do specifically with window, and happens when testing for the presence of any global. window is just the most common case, because it's frequently tested for to determine whether we're running in a browser.

It's very easy to imagine analogous cases, though I confess I have no recent real-world one at hand. I think testing for some browser plugins used to sometimes use this, but global namespace pollution concerns made it now rare or extinct.

[ian-craig]

Testing window in code which also runs in server side rendering is a common case I've hit. I can imagine the same would apply when conditionally adding a polyfill only when the browser does not already have support.

declare var fetch: any;

if (typeof fetch === "undefined") {
    // Insert some polyfill
}

[HamletDRC]

OK, I did not know that server side JavaScript would throw an exception if you referenced an undefined global variable.

[estaub]

@HamletDRC, In your browser dev tools, open a console and type

dfsdfdsfsdfgsdfberg

and press enter - you'll see that it's the same there. Be sure to spell it right ;-)

[HamletDRC]

OK, I had no idea.

I wonder if there is an easy way to determine if something is global or not.

[estaub]

@HamletDRC That's easy! If you don't know where it came from, it's global.
In other words, if there's no defining import, const, var, function, etc. in the module, then it's global by process of elimination.

[estaub]

Sorry, my last comment was thinking from a Javascript perspective, which may not be relevant in Typescript, since the dev provided a declaration somewhere, e.g. declare var window: any;.

In the case of a global, there's no javascript backing the declaration, but I expect that you have no way of detecting that if the declaration is imported. TSLint just does static analysis of the Typescript source, right?

[estaub]

I'm not sure, but I think something is global iff it is declared (using declare) outside of a namespace, and is not exported. Don't trust this, though; it needs verification from someone with more Typescript savvy.

If any Typescript mavens see this, please confirm or deny.

[bradleyayers]

I've been using no-typeof-undefined for a while, but I'm thinking of dropping it since its value seems questionable to me. In some cases foo !== undefined throws a ReferenceError, but typeof foo !== "undefined" will never throw. Using typeof seems like a safer convention.

I'm curious to hear what value others see in no-typeof-undefined to use it?

[estaub]

@bradleyayers
See issue #139: it's "simpler".
I wonder if it was driven by some external authority.

Related: https://stackoverflow.com/questions/3390396/how-to-check-for-undefined-in-javascript

Could be wrong, but I believe the cases where, in Typescript, x === undefined can throw a ReferenceError (without a compiler error) are pretty limited, maybe even limited strictly to the declared-but-not-defined-global case discussed above. So I'm happy to use it everywhere else.

[bradleyayers]

@estaub thanks for the link! I've written up a quick proposal for a new TypeScript feature related to this microsoft/TypeScript#23602.

[JoshuaKGoldberg]

Adding a missing or so as per the TS feature proposal would be great and absolutely solve the problem discussed here.

That will almost certainly take a very long time. Until then, accepting PRs to allow the user to add a whitelist of allowed variable names, e.g. ["global", "window"].

[JoshuaKGoldberg]

☠️ It's time! ☠️

Per #876, this repository is no longer accepting feature pull requests. TSLint is being deprecated and we recommend you switch to https://typescript-eslint.io.

Thanks for open sourcing with us, everyone!