no-var fix isn't safe in the global scope

[Jamesernator]

I'm not sure what the project's policy is about safety of fixes, but the no-var can produce unsafe transformations when the var is global which makes it unsuitable for fixing arbitrary files.

This is a minimal example that isn't safe (and here's a demo page link to it):

var x = 10;

When fixed by eslint it produces:

let x = 10;

However this isn't equivalent as in the global context let/const statements have different semantics than var, take this code for instance:

<script>
    var x = 10;
</script>
<script>
    var x = 20;
</script>
<script>
    console.log(x);
    console.log(window.x);
</script>

Running that code will print 20 twice, but in contrast if we run the eslint "fixed" version:

<script>
    let x = 10;
</script>
<script>
    let x = 20;
</script>
<script>
    console.log(x);
    console.log(window.x);
</script>

In this case it will print 10 followed by undefined, in addition the second script will simply throw an error that x is already defined.

Now I'm not planning on using globals in this way in new code, but a lot of files in the code base are many years old and I know I've seen var someGlobalName being used to add things as window.someGlobalName so running the no-var on the code base is very likely to break things. It would be nice at least to have an option to no-var like ignore-global or something like that to prevent breaking of such code.

[VictorHom]

Hey @Jamesernator

Thanks for submitting an issue. Definitely a good point.

With regards to the global variables, and how to ignore, ESLint has ignore lines (https://eslint.org/docs/user-guide/configuring#disabling-rules-with-inline-comments) which could solve the issue if there are only a few globals floating around. Would this work for you?

The other part is whether there can be an option that takes in a list of var variable names to ignore those variables and not fix them. Is that correct?

[not-an-aardvark]

I think the best thing to do here would be to continue to report top-level global variables, but not autofix them. The fact that autofixing can cause code to break here seems like a bug to me.

[Jamesernator]

@VictorHom I'm not actually writing new var someGlobalName declarations, it's just the case that when running eslint with fix on existing files in the code base it might break files that contain those declarations so added comments doesn't work as I don't know what the contents of all files might be. @not-an-aardvark's solution is exactly what I want anyway so if that happens that would be ideal.

The option I was suggesting was more a workaround so that you could tell eslint to ignore top level vars:

/* eslint "no-var": [2, { ignoreGlobal: true }], "prefer-const": [2] */
var someGlobalVar = ...

function foo() {
    var localVariable = 10
    ...
}

// Would become:
var someGlobalVar = ... // Not changed as it breaks

function foo() {
    const localVariable = 10 // Changed as it's safe
    ...
}

Now one thing that might be a bit annoying about removing the autofix for global vars is that people might be using it with CommonJS instead of the browser in which case it is safe to autofix global var declarations, so perhaps it'd be better for some option like environment which could be set to hint that autofixing global var declarations is safe.

E.g. maybe something like:

/* eslint "no-var": [2, { env: 'commonjs' }] */

var x = 10; // Will get fixed because it's safe to fix within commonjs

// ---------

/* eslint "no-var": [2, { env: 'browser' }] */

var x = 10; // Won't get fixed because it's not safe to fix in the browser context
           // but it would be nice if it were still reported 
           // to be fixed manually

[platinumazure]

I'm 👍 to disabling the autofix logic for global var declarations.

[not-an-aardvark]

We could probably tighten that constraint so that we still autofix top-level var declarations when using sourceType: module or ecmaFeatures: { globalReturn: true }, because in those scenarios no global variable is created.

[mysticatea]

I'll work on this.