Forbid using window properties as global variables

[sidoshi]

All the properties of window object such as name, self, and status are considered to be global variables because of having browser env in eslint config.
As a result this variables are ignored by eslint for errors such as no-unused-vars when someone uses this variables in their code without the intent of using them as a global variable.

To solve this we'd have to remove browser env from eslint config and add a list of global variables that are more commonly used. Other window properties are not considered global and requires window. qualifier.

I have added a list of all variables that I think are commonly used as globals in browser env.
Please guide if I have missed something.

How did I test this?
I created a new app with
npm run create-react-app test-app
npm start
Added console.log(status) to index.js file
And it showed error in terminal that status is not defined. (As expected)

But text-editors (Atom) won't show this as it requires eslint-config-react-app to be installed globally and hence it becomes necessary to update that package globally for this to work.

closes #1834

[n3tr]

Are there any common global variables need to be added (such as requestAnimationFrame, XMLHttpRequest)?

I think we can use globals for reference, As I know eslint also use it for its environments.

What do you think?

[sidoshi]

But I am not sure how to determine what variables are more common than others.
Eg: I thought we would want location to be used with window qualifier but it breaks CI as the code uses location without qualifier.
So how do I determine which variable should be used as a global?

[langpavel]

Simply, use all window variables with window accessor. Isomorphic app developers will have less trouble!

[fhelwanger]

What about these?

• setTimeout
• clearTimeout
• setInterval
• clearInterval
• requestAnimationFrame
• cancelAnimationFrame
• requestIdleCallback
• XMLHttpRequest
• fetch
• console
• btoa
• atob

I never prefix these with window., and I'm certain that I forget others that people often use directly.

Just my 2 cents, but I don't think maintaining another whitelist of global variables is worth ¯\_(ツ)_/¯

Anyway, maybe this list can help, it's what eslint uses.

[fhelwanger]

I just remembered about some classes that I have seen people use without the window. prefix:

• FileReader
• FormData
• Worker
• SharedWorker

[sidoshi]

Maintaining this list doesn't seem like best solution as there are too many variables and we can't determine what should be used without window prefix.

[sidoshi]

But there are too many general variable names in window properties which are considered as globals and ignored by eslint😑.
I think we should blacklist variables that should not be used without window prefix.

[fhelwanger]

I agree, blacklisting seems much simpler.

[langpavel]

Whitelist, please. Think about server side rendering.

• console
• setTimeout
• clearTimeout
• setInterval
• clearInterval

[gaearon]

Standard allows only document, console, and navigator without window qualifier. I think it's a reasonable list.

setTimeout and friends aren't affected because they're part of the language.
rAF is affected but is not very commonly used so I'd be fine with using window. for it.

[gaearon]

Let's change this to the list of globals defined by Standard: document, console, and navigator.

[langpavel]

@gaearon document and navigator are not part of node, so it still can bring headache to SSR app developer.

[gaearon]

We need to draw the line somewhere, and I think it's honestly not hard to find usages of them if you intend to convert your app to SSR (which CRA doesn't support out of the box anyway).

It's much harder without a whitelist (since then you don't even know what you're looking for).

[sidoshi]

Also we need to add window in globals because without browser env, eslint does not consider it as global.

[fhelwanger]

@gaearon I don't think setTimeout and friends won't be affected by default. Unless they are specified in the whitelist, eslint would complain about them.

I created this repo to reproduce it, I hope it helps.

I also included some other commonly used variables, like Worker and fetch.

I really wouldn't like to have to convert my code to add window. before every fetch call, for exemple.

[fhelwanger]

@gaearon oh, sorry. setTimeout won't be affected because the node env is also included, my bad 😁 I updated the exemple above to fix it.

But anyway, other things like rAF, (session|local)Storage, fetch and Worker still would produce errors. I really think they should be allowed without window..

[sidoshi]

@fhelwanger setTimeout won't work if tested in isolation. But as we have node env, setTimeout and others are included in it.

[fhelwanger]

@doshisid yeah thanks! sorry about that, just updated the example #1840 (comment)

[sidoshi]

I think with the exception of fetch requiring window prefix would be much cleaner.

[fhelwanger]

I really don't see much code prefixing Worker and rAF with window., but maybe I'm biased ¯\_(ツ)_/¯

Anyway, what I liked about CRA is that it doesn't enforce a code style, and I think requiring this window prefix is some kind of code style in these cases.

That is what I'd like to point out, if you disagree then it's fine 😁

Changes have been made

[Timer]

Looks good, thanks!

[Timer]

@gaearon per your comment in the export render() issue, we may want to revert this if we're not going to explore SSR in future releases -- imo this is more of a step towards SSR than fixing the original problem

[gaearon]

I agree. I’m afraid I am changing my mind here. (Sorry @doshisid for the trouble! Your contributions are appreciated. Sometimes it takes time to figure out what we need.)

I saw a lot of confusion about how to use globals, so I think we should limit the impact of this to as little as possible.

The alternative strategy I suggest is to read through the list of all browser globals, and only forbid lesser-known ones that can reasonably refer to local variables. For example status is a great candidate.

[sidoshi]

IMO this helps even if we are not going to explore SSR. See eslint/eslint#3959 (comment) .
But in any case, I am happy to help😊

[gaearon]

I agree with sentiment in that issue—my proposal is we go through the list and only exclude confusing globals. Needs some human curation but IMO this is okay.

[sidoshi]

Sorry I misunderstood your suggestion earlier. This seems good. Let me know if I can help.

[gaearon]

@doshisid If you could make another PR that blacklists all confusing globals from ESLint browser environment that would help! Now, “confusing” is subjective but I think we can agree that if you can reasonably name your variable this way (e.g. top or self or status), it probably should be disallowed as a global.

[sidoshi]

To blacklist variables, AFAIK we will have to still remove browser env and add all the browser variables to globals with the exception of the variables we want to blacklist. Correct me if there is a better way to do this.

[gaearon]

Yes, this seems fine. I just mean that you can look at ESLint source to find a definitive list of which globals are defined in browser, and use that as a starting point.

[satya164]

Maybe it makes sense to write a custom ESLint plugin which blacklists some names rather than maintaining a whitelist? I have no experience in ESLint plugins so disregard if it's a stupid idea.

[sidoshi]

I don't think we can blacklist variables using plugins. We'd still have to maintain a whitelist.
But if we export whitelist from a plugin as an env, it would make the main config a lot cleaner.

[epmatsw]

The list of globals that eslint uses for the browser environment is pulled from globals

[satya164]

Sindre has a module for everything imaginable!

[gaearon]

We went with #2130 instead.