Dangerous "name" (and potentially others) global

[pocesar]

TypeScript Version: 2.6.0-dev.20170913

Code

if (name === "asdf") {
  console.log("asdf")
}

https://www.typescriptlang.org/play/#src=if (name %3D%3D%3D "asdf") {%0D%0A console.log('hello')%0D%0A}

Expected behavior:

Error out

Actual behavior:

More than once, I had a "local" variable called name, outside a block scope, and it didn't warn me that it's not defined (or used before declaration). even though, in my code, event.name is a string, the global name variable is of type never. it's defined in lib.dom.d.ts, along with other "juicy" global names that will provide some confusion, like length, external, event, closed

one 'fix' (that would be a breaking change) would to make lib.dom-globals.d.ts (the propertiers that are usually accessible through window variable) and make it an opt-in in tsconfig.json compilerOptions.lib array

[kitsonk]

Duplicate of #9850

[pocesar]

that didn't fix the problem. you can still use the variable and the nightly will happily use it without complaining, so not really a duplicate, but a regression

the reason for an opt-in is because some typings need "DOM" declarations (like pouchdb), and even if you're using node, you need to use "lib.dom", just because you need the Blob declaration.

[RyanCavanaugh]

We keep going around on this and need to come up with something that really solves the issue

[DanielRosenwasser]

Spoke with @mhegazy - let's change it to void so it's not comparable to anything?

[pocesar]

how about a tiny breaking change for 2.6 and separate the global DOM declarations into another lib file? is it feasible?

[mhegazy]

how about a tiny breaking change for 2.6 and separate the global DOM declarations into another lib file? is it feasible?

u can do this today with --lib es6 that does not include the dom.

[pocesar]

but I have typings that rely on DOM stuff, even if it's not actually used in node.js (like pouchdb), but there are many other isomorphic typings

[kitsonk]

Well then you include the DOM library... It doesn't make any sense to have a partial DOM library, or even a globals DOM library... Because I am assuming you aren't suggesting that the global window and document get moved to this separate global DOM declarations lib file. Why only half model something? That sounds very dangerous.

[pocesar]

properties of "window" that are considered globals is there, under window global. you shouldn't be using globals anyway. how is that dangerous? you should be using window.name and not name anyway. or window.menubar instead of menubar. classes(ish) are properly named, like Blob, Uint8Array, JSON, and you know you won't be crossing with those by mistake. if you DO want to use browser globals like it's 1999, it's up to you to include the legacy way of accessing global variables that are actually under window

[kitsonk]

if you DO want to use browser globals like it's 1999, it's up to you to include the legacy way of accessing global variables that are actually under window

Not using them doesn't make them go away... So you are suggesting that the libraries not reflect the actual environment?

[pocesar]

not sure if trolling or serious.

[kitsonk]

You implied breaking out the DOM definitions. Mohammed told you how you can do that today. You then implied that wasn't good enough and seemed to imply something else, like that the global you don't like be located in a separate library file. Why do that, or more specifically what are you suggesting, because it appears to make no sense to me.

[RyanCavanaugh]

void is a slightly safer type than never, so we'll change this. Should also modify length since it's also problematic

[falsandtru]

Can you remember #15424? And I guess string | void is better than void.

[RyanCavanaugh]

string | void will still allow string writes

[mihailik]

Should it be never | void?

Aside from anything, it sounds REALLY cool — might sell some T-shirts and other memorabilia.

[falsandtru]

Certainly string | void is not perfect for type safety of local variables but it has a good balance between type safety and usability of global name variable. So I think it is also reasonable.

[mihailik]

@falsandtru the global name can be accessed/assigned via typecast with little to no fuss. TS even elides any extra brackets.

Signalling up both name and length would be really helpful in real life code: people do make those errors. Creative declaration for name is a cheap way to get there, alternatively the compiler may have a special handling just for these specific globals.

[falsandtru]

name: void is also acceptable for me.

[falsandtru]

A patch exists here: microsoft/TypeScript-DOM-lib-generator#240

[mihailik]

Simple suggestion: remove name and length declarations altogether. That will error on any use of name/length in global scope, and with a very obvious error message.

Playground link

function use(obj) {
    ñame = obj.name;
    ~~~~
    Cannot find name 'ñame'.
}

Declare name and length in your code if you really need it. Whoever wants to shoot themselves in the foot — they bring their own gun.

[mihailik]

Besides, look at this misleading error you get in current TS:

myname = 'Allen'
~~~~~~
Cannot find name 'myname'. Did you mean 'name'?

The existing declaration trickery pushes people into using name global. Not ideal.

[RyanCavanaugh]

@mihailik insufficient because you can still do this

var name;
if (someCond) { name = "hello"; }
if (name !== undefined) { 
  // unreachable

[pocesar]

just got bitten again by "status" (that comes from Promise.allSettled callback), it's a global variable...

[telamonian]

I'll add my voice to the chorus here. There's a line in my code that's used to fetch icon objects based on icon names:

const resolved = LabIcon._instances.get(name);

where name is a local variable that stores icon name. Just now I refactored name => icon, but forgot to change the line above. And then all of the icons in my app broke.

Took me 10 minutes to trace it back, and then another 10 minutes of googling to find out why my compiler hadn't noisily failed/to find this thread.

[hyurl]

name variable comes from browsers (DOM), so if the --lib compile option doesn't contain any DOM version, allowing name to be available is definitely a BUG, I don't know why the core team just can't admit it. I don't believe anyone would potentially use this variable in Node.js environment without knowing what he is doing, concerning about losing compatibility of removing this variable is unreasonable.

PS: this issue had been open since 2017, it's now 2020, even if someone accidentally used the variable in history, it should be all faded out by now.

PPS: new versions of TypeScript did break some compatibilities, for instance, the IterableIterator interface, I don't see anybody complaining about it, people just think TypeScript is getting better and more accurate.

[tomboland-vocovo]

I've just been bitten by this, and I'm not even writing TS, I'm using VSCode's type-checking support for plain JS. I was very suprised to find status was seemingly completely OK, due to being defined in lib.dom-globals.d.ts, and I introduced a bug where I should've been referring to statusCode defined a couple of lines above!

[thatodieguyspj]

The current functionality has allowed some bugs in our codebase. "name", "length" & "event" being global variables seems like ancient technology to me. Is anybody using TypeScript and still accessing these?

Here's our workaround using an ESLint rule:
"no-restricted-globals": ["error", "closed", "event", "fdescribe", "name", "length", "location", "parent", "top", ],

[joshuakb2]

This just got me in a node project. ReferenceError: name is not defined. I'll be using @thatodieguyspj's ESLint rule, thanks for that!

[pikeas]

Experienced dev new to Typescript, just bit by this. I have no position on how this should be improved, but the current behavior is a bad dev experience to the point where it should absolutely be changed.

Names like, well, "name", are extremely common; it's a massive footgun for code to compile successfully and then error at runtime due to what is substantially an undefined variable, even if this global type definition makes sense in the historical context.

[hyurl]

If anyone would argue that changing this may lead to unexpected errors for historical projects, I'd like to say that those projects are buggy by themselves.

[Jack-Works]

We keep going around on this and need to come up with something that really solves the issue

@RyanCavanaugh

With PR #40402, we can type those variables as declare const name: throw "using of global deprecated 'name'" and once people use that accidentally console.log(name), it will immediately generate a diagnostic instead of resolving to a never type. How do you think?

[Jack-Works]

#40468 but it requires changes in libdom.d.ts and not easy to i18n

[pocesar]

@sandersn this shouldn't be closed, there are many globals that should be changed

[orta]

I'm cool with keeping it open (it auto-closed) for discussion about the rest of the values, name I think is definitely the worst-case offender in there. Also throw types are cool.

[david-fong]

how about a tiny breaking change for 2.6 and separate the global DOM declarations into another lib file? is it feasible?

For anyone interested, discussion on ideas for addressing globals in lib.dom is the main topic of discussion in #14306 :)

[thw0rted]

Thanks for the pointer @david-fong . IMHO this issue should be closed in favor of that one, or, frankly, both should be closed as wont-fix. I think the eslint rule fully solves the actual problem, which is that implicit window scope is one of The Bad Parts of JS -- valid, but should be discouraged. That places it squarely in the wheelhouse of a linter, rather than a TS compiler option.

Look at discussions on other issues that want to catch code that has a bad smell but isn't actually wrong, like #13778. When you add a check like this to the compiler, you're going to either break a lot of code, or put it behind a disabled-by-default flag. New flags will cause a split, where some packages were compiled with it turned off, so now consuming code has to either bug maintainers to change their projects to comply with a flag they didn't ask for, or turn on skipLibCheck (because there's no way to opt-out of checking for a single dependency).

tl;dr: this is better addressed by a linter rule, which is already available.

[david-fong]

Those are great points. I didn't think of that. I don't think the lib.dom-strict option mentioned in that discussion would suffer from those problems though- would they? What I like about it is that it seems like it would be very simple to implement in the ts lib generator.

I think it would be great if you paste your comment into #14306 along with your thoughts on the lib.dom-strict option.

[thw0rted]

I'm not an expert but I think if you fork lib.dom, or split it such that some people are using "bad parts" of the global namespace while others aren't, you open yourself up to the same issues. Let's say I depend on some-lib and it includes a call to doStuff(name), intentionally referring to window.name using shorthand. They built their lib using an older version of TS, or they consciously included lib.dom-strict.d, but your consuming project avoids including the "strict" lib. Without skipLibCheck, the typechecker is going to descend into your dependency and flag the reference to name as an error.

[david-fong]

Let's continue this discussion in 14306.