Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Variable usage issues caused by global Window object #1351

Closed
ttsiodras opened this issue Dec 3, 2014 · 26 comments
Closed

Variable usage issues caused by global Window object #1351

ttsiodras opened this issue Dec 3, 2014 · 26 comments
Labels
Breaking Change Would introduce errors in existing code Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Fixed A PR has been merged for this issue Help Wanted You can do this

Comments

@ttsiodras
Copy link

The example below compiles - it shouldn't, I think...

ttsiod@avalon /home/ttsiod/tmp/TypeScript.bug
$  cat foo.ts
function whyTSwhy(config: {
    name?:string
})
{
    console.log(config.name);
    console.log(name);
}

ttsiod@avalon /home/ttsiod/tmp/TypeScript.bug
$  tsc -t ES5 foo.ts 
$

Typescript fails to see that there is no "name" variable - only "config.name" is valid in this context.

I am using the version from the repos, commit af4a121 .

@ttsiodras
Copy link
Author

I figured out why this happens - and I think the TypeScript compiler needs, at the very least, a warning here.

Apparently, all the global Window properties are visible "as is" - which means that using them inside functions is automatically "resolved", even if their use is a typo or an error (as it was in my case).

If I may suggest, please add a warning when compiling .ts code that uses these "globals" - e.g.:

... bug.ts(6, 10): Warning: you are accessing the global 'window.name' here - to remove this warning, change 'name' to 'window.name'.

@NoelAbrahams
Copy link

This is a common problem and it would be nice to have a solution.

See #494 and #983

@RyanCavanaugh
Copy link
Member

We don't actually add all the members of Window to the global namespace; name (and other globals) are just declared twice (once globally, once in the Window interface).

I am inclined to say we should remove the name and length properties from the global declaration space as they are very likely to be used intentionally. It would be a breaking change but I don't think people actually use those properties in an unqualified way (on purpose).

@RyanCavanaugh RyanCavanaugh added Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript labels Dec 3, 2014
@mhegazy mhegazy self-assigned this Dec 3, 2014
@mhegazy mhegazy added this to the TypeScript 1.5 milestone Dec 3, 2014
@NoelAbrahams
Copy link

I am inclined to say we should remove the name and length properties from the global declaration space as they are very likely to be used intentionally.

👍

Other common sources of error include location, parent and event.

I've also had problems with document but that is likely to be used unqualified intentionally.

@DanielRosenwasser DanielRosenwasser changed the title Failure to detect use of non-existing variable... Variable usage issues caused by global Window object Dec 5, 2014
@mhegazy mhegazy modified the milestones: TypeScript 1.5, TypeScript 1.6 Feb 5, 2015
@mhegazy mhegazy added the Revisit An issue worth coming back to label Apr 22, 2015
@Arnavion
Copy link
Contributor

I don't think it should be removed. That would allow writing code like

var name = [1, 2, 3]; // Expect name to be number[]
name.forEach(...); // Blows up
console.log(name === "1,2,3"); // true - it's actually the string "1,2,3" because it does set window.name which stringifies the array

Edit: Or even with a string name, it'd give the false impression that it's a separate entity from window.name when it actually isn't.

@zhengbli
Copy link
Contributor

zhengbli commented Mar 2, 2016

With the library modularization work at #6974, we may have an option for --lib called DOMWithoutGlobal which doesn't include the global variables. However, the discoverability of such an option may be an issue.

@mhegazy
Copy link
Contributor

mhegazy commented Mar 2, 2016

node users can ignore the DOM, but web developers will still run into these inadvertently.

@Avol-V
Copy link

Avol-V commented May 25, 2016

About window.event

I think it's not really same, as topic starter's problem — name property is standard. Yes, it can also lead to misunderstandings, but event property is just trash in global space, and didn't work in all major environments.

“present in many environments” is not seems to me as an argument — it's non-standard property from old times. In such case you can add attachEvent function, why not?

There is a lot of really useful properties, that didn't exist in TypeScript definitions. For example, when I need to control css-property resize I had to extend global object (resize present in many environments) like this:

declare global
{
    interface CSSStyleDeclaration
    {
        resize?: string;
    }
}

If somebody really wants to use event as global property, they can do similar thing.

I'm sorry if my message will seem harsh, but it seemed to me that TypeScript tends to follow the standards, not to describe some specific implementation.

This is not a complaint, just my thoughts. And thanks for your work — TypeScript is a great language!

@NoelAbrahams
Copy link

The solution we've gone with is to specify --noLib and define your own global variables (borrowed from lib.d.ts). This is probably the best way to gain control of the global namespace.

@Seikho
Copy link

Seikho commented Jun 27, 2016

Should window and all of its associated globals be part of the default lib.es5/es6.d.ts/lib.d.ts type definitions? Not all projects are Node projects and not all projects are browser projects. I believe the browser/window type definitions should be optional and treated in the same way as Node is.

@yortus
Copy link
Contributor

yortus commented Jun 27, 2016

I agree with @Seikho. ECMAScript is host-neutral. W3C specifies web APIs for standard browser hosts, The Node.js Foundation specifies the node.js host environment. I don't think either belong in the default lib.d.ts. Specifically lib.es5.d.ts, lib.es6.d.ts, etc bundle browser-side web APIs, even though they are not part of any version of the ECMA-262 standard.

The OP's problem arises because the default lib.d.ts declares all the Web APIs, thus treating all environments (including node.js ones) as if they are browser environments, which does not reflect the runtime reality. A node.js project written in TypeScript can reference all Web APIs, everything from name to requestAnimationFrame to HTMLCanvasElement, without a compiler error (but obviously with a runtime error). As TypeScript's type system becomes ever-more expressive and accurate, I think this approach looks increasingly 'loose' and unacceptable.

I'd prefer that the browser host environment was in a separate opt-in lib.web.d.ts (or similar), much the same as the node.js host environment is in a separate opt-in node.d.ts. That way, both browser and node.js projects may opt-in to their own host environment without any global pollution from the other environment.

The team recently decided not to bundle node.js typings. How about unbundling the web API typings too? That would make TypeScript host-neutral like ECMAScript is, and put node and browser projects on an equal footing.

@Arnavion
Copy link
Contributor

@Seikho @yortus Note that as @zhengbli said, this problem is already solved for non-browser users with the --lib compiler option.

@yortus
Copy link
Contributor

yortus commented Jun 27, 2016

@yuit can you confirm @Arnavion's above comment? Looking at the diff for 6974, I can see that lib.d.ts still bundles in all the Web APIs, and I can't see any DOMWithoutGlobal option for lib.

How do I target, say, an ES6 node.js environment, without including all the browser-side typings?

@yortus
Copy link
Contributor

yortus commented Jun 27, 2016

Confirmed it myself. Adding "lib": ["es6"] to the project's tsconfig.json includes just the ECMA-262 definitions (Set, Map, Promise, etc) but none of the Web APIs (window, name, etc).

Adding "lib": ["es6", "dom"] includes both the ECMA-262 typings and the Web API typings.

So, is there any reason why this issue remains open? It seems no longer a problem. What other changes are needed?

EDIT I'd say discoverability is still an issue. tsc includes all Web APIs in the build by default, and you have to opt-out by specifying something like "lib": ["es6"], which is not obvious.

@myitcv
Copy link

myitcv commented Jun 27, 2016

@yortus the "solution" to this thread is not the --lib option per se, because the "dom" option still pollutes the global namespace with variables like name (and others) (see this comment from @RyanCavanaugh)

@Avol-V
Copy link

Avol-V commented Jul 6, 2016

And I once again draw attention to what is now the library has an error — property event that is absent in the standard.

@lefb766
Copy link

lefb766 commented Aug 18, 2016

The problem with global 'name' is going to be fixed (#9850).

@mhegazy mhegazy modified the milestones: TypeScript 2.0.2, Community Sep 2, 2016
@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Sep 2, 2016
@mhegazy mhegazy closed this as completed Sep 2, 2016
@isuda
Copy link

isuda commented Jan 9, 2017

@mhegazy - This issue as stated in the original post is not fixed.

I can still do console.log(name); even if name is type never.

Even worse, I can now do stuff like this:

let someNumber: 123;
someNumber = name; // this is no longer a compile error.

@mhegazy
Copy link
Contributor

mhegazy commented Jan 9, 2017

name does exist in the global scope if you are using DOM. so we can not remove it totally.

@lefb766
Copy link

lefb766 commented Feb 5, 2017

@mhegazy I'm not sure what you meant by "remove it totally".

My understanding is that you are saying we cannot remove name and others from the global because people can still re-declare them to get runtime error.
That's why #9850 was applied, wasn't it?

However, we could argue that typing name as never is not sufficient.
As @isuda pointed out, the never fix won`t catch everything.

Basically, using never as "do not use" label is a misuse.
never type stands for "never reach at runtime" or something.
Then, passing around never value will not be compile error ("never is a subtype of every type") because these code isn't thought to be executed.

So we can do better.
An optimal solution can be introducing another special type prohibited, which any usage of that type except variable declaration raises an error.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Breaking Change Would introduce errors in existing code Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Fixed A PR has been merged for this issue Help Wanted You can do this
Projects
None yet
Development

No branches or pull requests