getify / eslint-plugin-proper-arrows Goto Github PK

Reasoning:

• Usually clear from context which ID is being referenced.
  • Rule could be be interpreted to say that a name like userId is preferred, but inside a function handling user logic that might be redundant.
• people use the term ID rather than Identifier when talking
• Even if the type of ID is unclear, it is still clear what an ID is. That sort of code review seems beyond the scope of a linter.

Issue

Running the command eslint . all the rules are notified properly. But when using ESLint on text editors such as VSCode and Atom for instance, some warnings/erros are not notified:

"where" rule:

// not notified cases
export const a = () => {};

// or

const b = () => {};
b();

// notified
export default () => {};

I changed some tests, adding this cases:

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

	var results = eslinter.verify( code, linterOptions.whereGlobalDefault );
	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" );
} );

✅ The test above passes but this warning/error is not notified on the editor.

QUnit.test( "WHERE (export): violating const", function test(assert){
	var code = `
		export const a = () => {};
	`;

	var results = eslinter.verify( code, linterOptions.whereExport );
	// HERE THE RESULTS VARIABLE IS AN EMPTY ARRAY []
	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, "noExport", "messageId" );
} );

❌ The test above breaks because results variable is an empty array. But this case is notified while running on CLI.

Could someone help me understand what is the problem in these cases?

I was trying to add an export-only-named-function (do-not-export-arrow-functions) rule to my project and found this awesome ESLint plugin that would fit perfectly for my scenario. The problem is that I cannot add it if other developers cannot see it at development time. It would only notify on CI by the time they open a Pull Request, and it would be a bad experience to find the log and come back to the code to fix a linter issue 😞

More info:

Tested editors: VSCode and Atom

OS: Mac OS X Catalina (10.15.2)

Node: v12.13.0
npm: 6.12.0

My project is using ESLint v5.16.0

ESLint config:

// .eslintrc.js
module.exports = {
  root: true,
  env: {
    browser: true,
    es6: true,
    jest: true,
  },
  parserOptions: {
    parser: 'babel-eslint',
  },
  plugins: [
    '@getify/proper-arrows',
  ],
  rules: {
    '@getify/proper-arrows/where': 'error',
  },
};

Because some of us feel that chained arrow returns (such as are common in curried function definitions) is much harder to read, given that you basically have to read them right-to-left to figure out where the function boundaries are, add rule that forbids chained arrow returns:

var f => x + 1;   // not chained, OK

var g => x => y => x + y;   // Error

Also, we need a flag to optionally allow/ignore these:

var g = x => (y => x + y);
var g = x => { return y => x + y; }

Given code

const App = React.lazy(() => import('./SomeComponent'));

with config

    "@getify/proper-arrows/where": [
      "error",
      { global: true, property: false, export: true, trivial: false },
    ],

causes violation reported:
Arrow function not allowed in global/top-level-module scopeeslint(@getify/proper-arrows/where)

This rule disallows arrow functions in various locations (of course each is configurable):

• top-level/global scope
• object property
• export expression

The following will report a params count error:

({foo, bar, baz, bat, bongo }) => {/* a function that handles all those 'named' parameters */}

While the following won't

function namedParams({foo, bar, baz, bat, bongo }) {/* a function that handles all those 'named' parameters */}

This might be intentional, as it seems weird to use the pattern in an anonymous function, but the lint error should be more exact.

When an inline function expression appears in a default parameter value, it can create a closure over the normally-indistinguishable "parameter scope", which is super confusing, and should almost always be avoided.

For example:

var f = (x,cb = () => x) => { var x = 2; return [x,cb()]; };
f(5);   // [2,5]

The x parameter is in a separate scope from the var x in the function body, and in this case they are shown to have different values, via the closure.

For more info: https://gist.github.com/getify/0978136c0c66c0357f611e2c7233f105

This is confusing enough for regular functions, but it's significantly more confusing for arrow functions.

This rule would forbid inline function expressions in an arrow function's parameter default value position. It will have 3 modes:

• "all" - forbid all inline function expressions (arrow or normal)
• "closure" - only forbid inline function expressions that actually close over a parameter in the parameter scope
• "none" - disable the rule

There would also be two additional flags (both default to true), in effect only for "all" and "closure" modes:

• "arrow": forbids inline arrow function expressions
• "function": forbids inline regular function expressions

It'd be nice if we could include the functions x => f(x) under the definition of trivial. For example, if we want to call Array.map(x => f(x)) sometimes there are optional extra parameters of f that don't necessarily coincide with the parameters Array.map passes, and you need to delegate like this.

Another good example is for delegations that use methods, e.g. x => x.f().

Also constructors: x => new T(x)

[EDITORS NOTE: This issue was transferred from another repository, so it may no longer be valid, still TBD]

I personally can see merit to using anonymous functions using the shorthand syntax when doing simple calculations for filters and mapping arrays, for instance.

I would like the ability to only apply the rule when the arrow function contains curly brackets, so this should be fine

[1, 2, 3].map(x => x * 2).filter(x => x < 4);

But this wouldn’t be, without a reference to this

new Array(6).fill(undefined).map((_, key) => {
   let thing = DoSomething(key);
   return thing();
}

I’m in two minds as to what to do about returning objects:

x => ({
  …
})

I’ve never used this syntax because it’s too messy in my opinion, but I think it should be part of the rule options too, which may complicate things because of the brackets.

At the moment, the plugin is not allowing class property functions. Is this intended?
Would it be useful to have an option to allow this inside the no-name rule?

Would it be possible to ignore React components in the rule where? While I agree that function declaration is preferable when exporting a function (i.e. export function somefn() { /* ... */}), I would like to make exception for React components, because arrow functions are typed more easily.

When using function declaration, you have to type props and also return (from my experience, people usually forget to add null):

interface BoldProps {
    caption: string;
}

export function Bold({ caption }: BoldProps): JSX.Element | null {
    return <b>{caption}</b>;
}

Using arrow function is little bit more concise, less error prone, but still readable thanks to the React.FC type right after the variable name:

interface BoldProps {
    caption: string;
}

export const Bold: React.FC<BoldProps> = ({ caption }) => {
    return <b>{caption}</b>;
}

Thanks for the info.

Some people like to define arrows like this:

x => 1;
x => {};
x => void 0;

Those forms are all generally to avoid the () => ... But since this can be confusing to a reader to see a parameter listed that isn't used, this rule would forbid that.

It would also catch straight-up mistakes like:

(x,y,z) => x + y;   // z is unused here

This is similar to the built-in "no-unused-vars" rule, but is much narrower in focus: only for arrow functions.

Shouldeslint-plugin-proper-arrows work with NOOP ?

const NOOP = () => {};

While linting the following code, the plugin crashed:

export const clampData = data => flow(
    // Trim left side of the array
    dropWhile(([, y]) => y === null),
    // Trim right side of the array
    dropRightWhile(([, y]) => y === null),
)(data);

The error message:

TypeError: Cannot read property 'type' of null
    at getAllIdentifiers (./node_modules/@getify/eslint-plugin-proper-arrows/lib/index.js:628:12)
    at getAllIdentifiers (./node_modules/@getify/eslint-plugin-proper-arrows/lib/index.js:635:23)
    at exit (./node_modules/@getify/eslint-plugin-proper-arrows/lib/index.js:66:25)
    at listeners.(anonymous function).forEach.listener (./node_modules/eslint/lib/util/safe-emitter.js:45:58)
    at Array.forEach (<anonymous>)
    at Object.emit (./node_modules/eslint/lib/util/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (./node_modules/eslint/lib/util/node-event-generator.js:251:26)
    at NodeEventGenerator.applySelectors (./node_modules/eslint/lib/util/node-event-generator.js:280:22)
    at NodeEventGenerator.leaveNode (./node_modules/eslint/lib/util/node-event-generator.js:303:14)
    at CodePathAnalyzer.leaveNode (./node_modules/eslint/lib/code-path-analysis/code-path-analyzer.js:654:23)

Expected:
Skipping entries in an array is allowed (but perhaps not friendly) when destructuring. However, this entry will be returned as a null when iterating. A fix would probably entail checking for a null entry before checking the properties.

Extend "trivial" function definition to include an arrow that returns a void .. expression.

afaik eslint's max-params and no-unused-vars handle AsyncFunctionExpression

• https://github.com/eslint/eslint/blob/master/lib/rules/max-params.js
• https://github.com/eslint/eslint/blob/master/lib/rules/no-unused-vars.js

The current detection makes an assumption about global scopes which doesn't hold for when the script in question is being linted as if in a node/commonjs env, as they create an "extra" scope below the real global for your script's top-level scope.

Detecting this environment difference is quite tricky/nuanced, since env setup is not reported to plugins. But it turns out it can be detected by looking at context.parserOptions.ecmaFeatures.globalReturn, which if set to true, means the extra scope has been created.

This is either a bug or I'm unwittingly making a feature-request.

I've added eslint-plugin-proper-arrows to our preset and added the following code to its test-suite which I was expecting to fail:

# properArrowsWhere.invalid.js
const arrowFunc = (foo, bar, baz) => { console.log(foo, bar, baz) }

export default arrowExport => console.log(arrowExport)

I expect it to generate two errors on rule @getify/proper-arrows/where, but actually only get one error for the arrow-export. My eslint configuration:

    "@getify/proper-arrows/where": ["error", { global: true, property: false, export: true, trivial: false }],

From your readme I expected the arrow function to fail because it says:

"where": restricts where in program structure => arrow functions can be used: forbidding them in the top-level/global scope, object properties, export statements, etc.

Here top-level/global scope to me reads as the top-level in a module. But maybe that's not the intention of the library?

From debugging I think the change I expect could be satisfied by changing the line in https://github.com/getify/eslint-plugin-proper-arrows/blob/master/lib/index.js#L735 from:

scope.upper.type == "global"

to:

scope.upper.type == "global" || scope.upper.type == "module"

But I don't quite understand your test-suite setup (or indeed eslint in general :) so I'm not sure that's right.. and I'm doubly not sure if you intend for "global" to also mean top-level in a module at all. Maybe I'm actually making a feature-request to add a "module" toggle flag?

Either way I'm happy to try and help, but first need to reach out to you to understand the context of your library. Is this a bug at all?

(and thanks for making the library available)

var f = x => x > 10 ? x < 100 ? x : x / 2 : x + 1;  // ugh

This rule will have a depth level for ternaries... default to 0 meaning "no ternaries allowed", but 1 would mean one level of ternary, 2 would mean two (nested), etc.

Define one (or more) configs that make it easier to use this plugin out of the box.

Add a "configs" to the plugin definition: https://eslint.org/docs/developer-guide/working-with-plugins#configs-in-plugins

This rule would require parameter names to be of a certain length. It would default to requiring 2+ in length, but configurable to increase the required threshold to 3 or more.

I would like to forbid top level function declarations that use arrow syntax. This works using global.

const bad = () => {
  console.log('bad');
};

But I would like to allow top level arrow functions present within a function call. I cannot get this to work because these are considered global. I recognize they are indeed global. I would like to carve out an exception here. This issue here blocks me from being able to benefit from this plugin and I am going to have to disable it.

const things = [].map(x => console.log('eh, this is ok, it is in a function call');

Allow rules to auto-fix if possible.

and support as-needed and never

afaik async functions would behave the same as sync functions when it comes to inferred names

PS: I don't see why eslint's func-names shouldn't support asyncs, so if a PR gets accepted upstream, it's probably better than having a separate rule doing what func-names does but for async functions

I'm not sure if this rule exists already or not.

I find it hard to read arrow functions that return arrow functions like this:

var x = (a,b) => () => a + b;

I think having a more explicit return is easier to read.

var x = (a,b) => {
  return () => a + b
};

I am getting the following error when I add the rules to a CRA app and try and disable existing errors:

Line 1:1:  Definition for rule '@getify/proper-arrows/return' was not found  @getify/proper-arrows/return

Search for the keywords to learn more about each error.

The eslint-disable directive is as follows:

/* eslint-disable @getify/proper-arrows/return */

It would be nice to allow x => x < 0 and x => x > 0 as trivial functions, with other comparison operators too. These cannot ever throw and don't really need fancy names or parameters.

The boolean operators && and || should be allowed too IMHO.

Note that this does not include all arithmetic expressions, as for example x / y could still throw.

If you want to "train" a rules config on your existing code, a web based wizard tool could be helpful for that. You paste in code, it tells you what the maximum rules config is that would work (not throw errors) on your code.

Would just brute-force try all combinations of rules, in "decreasing" order, and print the first one that doesn't give any errors.

Don't allow stuff like:

var fn = (x,y) => (x = y * 2, y = x / 3, foo(x) + y);