Typescript: 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: https://github.com/Microsoft/TSJS-lib-generator/pull/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

[kitsonk]

Not to complain, but how would name being void in the global scope prevent that logic error? The local name would mask the global. I could see people getting confused with some sort of block scoped let or const with name that would be a problem, like:

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

[mihailik]

@RyanCavanaugh @kitsonk these are very synthetic cases may safely be left out of scope here.

The current never approach can be improved significantly by un-declaring name — that would cover more use cases than never, and provide much better errors.

If you're suggesting special-casing name at the compiler level, that's even better, but much much more work, testing than deleting a line.

[RyanCavanaugh]

This doesn't happen as often, but it's also bad to undeclare because name converts values to string on writes, i.e.

var name = 10;
if (name === 10) { // unreachable because name is "10"

[mihailik]

@RyanCavanaugh are we missing a point here? The proper 100% correct declaration for name is:

declare var name: string;

Anything different is a lie and has downsides. What I am saying cost/value is best for 'undeclare' lie:

• Very good error message
• Wide coverage of use cases
• Cases where it breaks are synthetic and unlikely in normal browser code

Comparing with either void or never there's more benefits and less downsides. You can special-case global name usage in compiler guts, but it's probably quite a lot of work?

Keep in mind, name is browser-specific. And even in browser it can be deleted, after which your examples would be actually fine: delete name; var name = 10; will actually store numerical 10 in name.

[ericanderson]

I just got bit by this:

const foo = "bar" + name;

Expected: compiler error
Actual: NBD

I dont know how else name can bite people, but I would definitely vote for removing it from lib.d.ts. Even if its technically correct, it is completely bad to have it this way.

If we are going to continue keeping it, perhaps things like adding a never to a string could be a compile error?

[thorn0]

Don't forget about the event global.

Just recently I've fixed a bug along the lines of:

events.subscribe(e => {
  if (event instanceof MyEventType) {
    // ...
  }
});

Please, let's simply get rid of them completely. Why keep them in the declarations?

[pocesar]

if someone is using Typescript to write legacy IE code, oh boy..... they should be slapped around a bit with a large trout

[ccorcos]

Can someone explain to me why we even need declare const name: never in lib.dom.ts? It makes no sense to me why we even have that.

[RyanCavanaugh]

@ccorcos Please read the thread; there are multiple examples of bad behavior if the name declaration isn't there

[thorn0]

@RyanCavanaugh On the other hand, it seems safe to remove event, doesn't it?

[testerez]

Maybe I'm naive but would it make sense to introduce something like a Forbidden type meaning that any use of the variable should raise a TS error?

[pocesar]

wow, just got bitten by this one again, this time on the server side

[testerez]

Now that unknown type has been introduced, would it be a solution to affect this type to window.name and other similar variables?

[DanielRosenwasser]

Yes! If you'd like to send us a PR, please feel free to make the change here and follow the appropriate instructions.

[pocesar]

but again, making it unknown is wrong (in the sense of being a real global name in browser scenarios). what is the problem again in having a lib.dom-globals.d.ts lib apart from lib.dom.d.ts? it's something specific that you'd manually add to your libs tsconfig.json

[mhegazy]

I would say just remove the declarations.. ppl are more likely to trip on them by mistake than they are to re-declare them at the global scope and expect them to work on web page.

[mihailik]

@mhegazy there was an argument for that a while ago: https://github.com/Microsoft/TypeScript/issues/18433#issuecomment-336393249

[mhegazy]

I think we are just trying to be too clever here. just remove the damn thing :)

[mboughaba]

Hello guys,

Any progress on this? I came across this issue today. Took me 15 minutes to figure it out 😢

Cheers,

[pocesar]

just happened again, this time with close global, tried to call it, it's declared in lib.dom.d.ts as declare function close(): void;

[hyurl]

In conclusion, the global variable name is actually a dom element comes from window.name, NOT in the js core. So, it's better to remove this variable from TypeScript when compile-option lib doesn't include dom.

[pocesar]

@hyurl some libraries are isomorphic, meaning they are made for the browser and node.js, so removing lib.dom.d.ts isn't something you can do at will

[hyurl]

@pocesar Such a library must explicitly include dom lib as I've tried, otherwise, the compilation will fail.

[aghoneim92]

I'm getting this with React 16, <Component value={name}/>. Why is the type checker not complaining that never is being assigned to string, even though my Component's props declare value as string?

[pocesar]

something weird happened just now, I had an event called "onSelect", but typescript accepted the global "onselect" (from window.onselect). another one that bit me. it's "null" by default, and my props were | null, took me a while to find this, because it was not showing any console.log statements, and since the same event was being used elsewhere, the "no unused locals" didn't help me spot it

[pocesar]

another one happened to me, my electron window was being closed after my react element was unmounting, I had this piece of code:

  const closeAll = useCallback(() => {
    while (unpipes.current.length) {
      const p = unpipes.current.shift()
      if (p) {
        p()
      }
    }

    while (conn.current.length) {
      const p = conn.current.shift()
      if (p && p.close) {
        p.close()
      }
    }
  }, [conn, unpipes])

  useEffect(() => {
    return close
  }, [close])

can you spot the issue? I couldn't until wasting 8 hours trying to fix "the unfixable". close is window.close, and was executing when my component unmounted. the closeAll function was named close before, because it was a single ref and not a ref of an array with registered cleanup callbacks...

on lib.dom.d.ts

vscode highlight

the funny part is that I'm also using tslint, and it went unnoticed

[ericanderson]

Perhaps we could get a compiler flag for noImplicitGlobalThis? Thus using name would be an error but if you want to use globalThis.name you could?

[NiclasHansson]

Forgot to destruct my variable 'name' from props and ended up using the global name variable instead. To me it's obviously a very bad idea to have globals with such common variable names as 'name' and 'length'.

Any plans on removing them and letting people access them through window instead? Or have they been already in later versions (I'm using v2.8.3)

[pocesar]

the issue is much bigger than it appears to be, since it can ship broken software. even with all the strict functions enabled, and using tslint as well...

[hyurl]

This is an obvious BUG, admit it

[inca]

I agree with this being a bug, even with name being defined as never one can still write code which compiles and results in runtime issues. Consider following examples:

// ------- 1 -------
interface Foo {
    foo: object;
}
const foo: Foo = {
    foo: name
};

// ------- 2 -------
function a(foo: string) {}
a(name);

// ------- 3 -------
function b(params: {foo: string}) {}
b({ foo: name });

All 3 compiles perfectly. All 3 should be compile-time errors instead (not linting errors!) because they are directly related to symbol/type resolution (which is compiler's domain).

[jchv]

I am still unsure why unknown wouldn’t be a better type than never.

[pocesar]

this issue missed the timing for a breaking change before wide adoption of TS... we have dom.iterable, that you must provide if you want to iterate using document.querySelector[All], but we don't have dom.globals...

[Kingwl]

Same on this one🤦🏻‍♂️

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