allow to detect only global this in arrow functions

[imsamurai]

hi, i added rule "allow" that can be used in conjunction with no-global to detect only arrow functions with global this. So for this case rule would be: "@getify/proper-arrows/this": [ "error", "allow", { "no-global": true, "trivial": false } ]

[getify]

I'm a bit confused by this. Can we clarify a bit which examples you want to have errors thrown, and which ones you want to allow to pass?

Here's what I think you're intending:

var f = x => this.foo(x * 2);         // ERROR
var f = (x = this.x) => x * 2;        // ERROR

var f = x => x * 2;                                      // OK
function f() { return x => x * 2; }                      // OK
function f() { return x => this.whatever(x * 2); }       // OK
function f() { return (x = this.x) => x & 2); }          // OK

Do I have that correct?

If so, here's where my confusion is coming in. This rule is already in the context of this, so it's weird (to me) conceptually to call it "allow" -- as in, "allowing ____" what? "allowing this"? Unless you've turned on "never" mode, this is already "allowed" everywhere -- this rule is only about making it required (or forbidden with "never"). So that seems redundant. Or the reverse: "allowing non-this"? Maybe that's the intent, but IMO it's not clear from just the word "allow"

Also, what happens if you set "allow" mode but don't turn on the "no-global" flag. Is that basically completely disabling the rule? I know many ESLint rules have something like that, but in my mind if you want that, you just don't enable the rule at all in the first place. Why have a lint rule with a mode where the lint rule does absolutely nothing!?

I think there may be a cleaner way to do this than having to combine "allow" and "no-global", which I hope avoids these confusions.

Perhaps we add just a single mode alongside "never" called "never-global", which does the same as "never" but only for global arrows. Just like "never" doesn't need "this" in it to clarify -- because the parent rule itself is only about this -- neither would this one need it. It should be clear that it actually means "never-global -this" (while still allowing, aka not saying anything about, non-this global arrows).

And like the "never" rule, "trival" and "no-global" flags would be moot and ignored in "never-global" mode.

[imsamurai]

Yes, your examples is exactly what i want. And such rule can't be reached with "nested", "always" or "never". Because "nested" and "always" require "this", but "never" require abscence of "this". My case that i saw today in my projects is after refactoring developer can just replace function to arrow function while there can be "this" inside. So major rule for me is to detect global "this" in arrow function, while absence of "this" is not an error. I agree that "allow" without "no-global" is useless, mea culpa)

If you agreed with "never-global" that acts same as "allow"+"no-global" in PR i'll make this changes. Also maybe we'll make this behavior for "never"+"no-global" (but it will break backcompatability, not good), what do you think about that?

[getify]

If you agreed with "never-global" that acts same as "allow"+"no-global" in PR i'll make this changes.

Yeah, I think this is the better approach.

I also would like to make sure we're verifying all the possible cases (such as the examples I provided above) in the tests.

Other cases to consider:

var f = x => y => this.foo(x + y);       // ERROR (nested)
var f = x => (y = this.foo()) => x + y;       // ERROR (nested)
// ...?

Also maybe we'll make this behavior for "never"+"no-global" (but it will break backcompatability, not good)

I don't think we should do that, especially since I don't think having two different ways to get the same behavior makes very much sense.

[imsamurai]

review please)

[imsamurai]

Take a look please. Too much if) Maybe you have better design or it's ok?

[imsamurai]

nestedThis will never be true shame to me)))

I made changes, but not exactly as you want.
I think separate never-global from other rules (if (neverGlobalThis) {) is cleaner. Also added one more test that covers nested global arrow with this

[imsamurai]

Hi! So you accept?)

[getify]

Thanks!! Released in ver 9.1.1.