Bug (?): Not identifying module-level arrow functions about eslint-plugin-proper-arrows HOT 4 CLOSED

Thanks for opening this issue!

not sure if you intend for "global" to also mean top-level in a module

Good question. To be honest I hadn't considered it either way. But now that you bring it up, yes I think having an arrow function in the outermost scope (whether that's the real global or whether it's the top-most scope of a module) should have been covered by the "global" setting.

So, yes, it seems like it is a bug (in the sense of unintentional shortcoming).

To fix, yes, I think these lines need to be updated:

https://github.com/getify/eslint-plugin-proper-arrows/blob/master/lib/index.js#L730-L735

We'd probably best change them to:

["global","module",].includes(scope.upper.upper.type)

// ..

["global","module",].includes(scope.upper.type)

And for tests, we'd need to add these config presets at the top:

whereModuleGlobalDefault: {
	parserOptions: { ecmaVersion: 2015, sourceType: "module", },
	rules: { "@getify/proper-arrows/where": ["error",{property:false,export:false,trivial:true,},], },
},
whereModuleGlobal: {
	parserOptions: { ecmaVersion: 2015, sourceType: "module", },
	rules: { "@getify/proper-arrows/where": ["error",{global:true,property:false,export:false,trivial:true,},], },
},

Then we'd need these four tests at the end:

QUnit.test( "WHERE (module-global, default): conforming", function test(assert){
	var code = `
		function foo() {
			var f = x => y;
		}
	`;

	var results = eslinter.verify( code, linterOptions.whereModuleGlobalDefault );

	assert.expect( 1 );
	assert.strictEqual( results.length, 0, "no errors" );
} );

QUnit.test( "WHERE (module-global, default): violating", function test(assert){
	var code = `
		var f = x => y;
	`;

	var results = eslinter.verify( code, linterOptions.whereModuleGlobalDefault );
	var [{ ruleId, messageId, } = {},] = results || [];

	assert.expect( 3 );
	assert.strictEqual( results.length, 1, "only 1 error" );
	assert.strictEqual( ruleId, "@getify/proper-arrows/where", "ruleId" );
	assert.strictEqual( messageId, "noGlobal", "messageId" );
} );

QUnit.test( "WHERE (module-global): conforming", function test(assert){
	var code = `
		function foo() {
			var f = x => y;
		}
	`;

	var results = eslinter.verify( code, linterOptions.whereModuleGlobal );

	assert.expect( 1 );
	assert.strictEqual( results.length, 0, "no errors" );
} );

QUnit.test( "WHERE (module-global): violating", function test(assert){
	var code = `
		var f = x => y;
	`;

	var results = eslinter.verify( code, linterOptions.whereModuleGlobal );
	var [{ ruleId, messageId, } = {},] = results || [];

	assert.expect( 3 );
	assert.strictEqual( results.length, 1, "only 1 error" );
	assert.strictEqual( ruleId, "@getify/proper-arrows/where", "ruleId" );
	assert.strictEqual( messageId, "noGlobal", "messageId" );
} );

Also, of course, the docs need to be updated in several places of these sections:

https://github.com/getify/eslint-plugin-proper-arrows/blob/master/README.md#rule-configuration-1

Probably change all references from "top-level/global" to "module-top-level/global" or something like that.

As a side question, now maybe that "global" name is confusing and should be changed to "outermost" or "top"? Not sure if it warrants changing, though. I guess most people will call a top-level module's scope "global" (or "module-global"). So probably don't need to change... but just something to ponder.

from eslint-plugin-proper-arrows.

Thanks!, that was above and beyond clear :D Seems like a nice and easy repo to work with, well done.

A choice had to be made: An export violation is also a module-top-level violation, as I can see it that must be true by definition. To alter the public interface as little as possible I opted for a change such that export violation is tested first, and if one is found then it is not also reported as a global violation. Happy to change it to any direction you prefer, this just seemed to me to be the most intuitive.

from eslint-plugin-proper-arrows.

An export violation is also a module-top-level violation, as I can see it that must be true by definition.

Good point, glad you brought it up.

I think the typical pattern I've used is that any overlapping rule is reported multiple times. In other words, a single line can have any number of rule violations applied to it. That's especially true since different rules might apply to different parts of the same line.

My reasoning was: it would be quite annoying to me if I found a line violating a linting rule, and then fixed that violation, only to have that same line now reported again for a different violation. Of course, this happens if my fix itself introduces the violation. But if the violation was hidden, I find that frustrating.

from eslint-plugin-proper-arrows.

An export violation is also a module-top-level violation, as I can see it that must be true by definition.

Good point, glad you brought it up.

I think the typical pattern I've used is that any overlapping rule is reported multiple times. In other words, a single line can have any number of rule violations applied to it. That's especially true since different rules might apply to different parts of the same line.

My reasoning was: it would be quite annoying to me if I found a line violating a linting rule, and then fixed that violation, only to have that same line now reported again for a different violation. Of course, this happens if my fix itself introduces the violation. But if the violation was hidden, I find that frustrating.

Yeah that makes sense. I'll update the PR.

EDIT: Done, great precise feedback thanks

from eslint-plugin-proper-arrows.