Testing: Add lint rule for path on Lodash property functions

[aduth]

Related: #6247

This pull request seeks to enhance existing custom ESLint rules forbidding the use of string paths for Lodash functions (see #6247 for context). It adds a few more functions which allow for path as an argument. It's suspected these were previously overlooked because for other functions, path was passed as the second argument.

For existing usage, instead of adding array wrappers, I opted to stop using the Lodash utility in favor of an equivalent arrow function. It uses a few more characters, but I think is a bit easier to read for those who aren't already familiar with the intent of the property utility function. This may be open to debate and I'm not strongly committed to keeping it this way.

Testing instructions:

Verify linting passes:

npm run lint

Ensure there are no regressions in affected runtime behavior: The updating of components which depend on viewport size (e.g. small screen requiring two-tap for select vs. interaction of a block).

[jorgefilipecosta]

Minor: An alternative would be to change the previous selector to include this cases e.g:

{
	selector: 'CallExpression[callee.name=/^(invokeMap|get|has|hasIn|invoke|result|set|setWith|unset|update|updateWith)$/] > Literal:nth-child(2), ' +
		'CallExpression[callee.name=/^(property|matchesProperty|path)$/] > Literal:nth-child(1)',
	message: 'Always pass an array as the path argument',
},

The main advantage would be that the message is not duplicated, but the selector looks complex and I'm not totally sure if this version is better or not.

[aduth]

Hmm, it's a good point. Since we're in JS here, I wonder if we could generate this to a single rule dynamically with a map:

[
	[ 1, [
		'property',
		'matchesProperty',
		'path',
	] ],
	[ 2, [
		'invokeMap',
		'get',
		'has',
		'hasIn',
		'invoke',
		'result',
		'set',
		'setWith',
		'unset',
		'update',
		'updateWith',
	] ],
].map( ( selector, [ argPosition, fnNames ] ) => (
	`CallExpression[callee.name=/^(${ fnNames.join( '|' ) })$/] > Literal:nth-child(${ argPosition })`
) ).join( ',' )

Or maybe too clever? 😆 Just trying to avoid the excessive duplication for what's effectively only varying on argument position and the function names.

[jorgefilipecosta]

Hi, I personally think this code not too complex and we are not being too clever. Also, my expectation is that most developers will not have to deal with this rules so I think this version is totally acceptable.
The only "improvement" I'm seeing would be to use objects instead of arrays, something like:

[ {
argPosition: 1,
functionNames: [ 'property','matchesProperty', 'path' ]
},

Maybe an object makes things even simpler.

[aduth]

In hindsight, an object would be much easier / clearer than working with the tuples, aside from minor detail of needing to pull in Lodash's map. Pushed updated in c9ab732.

[jorgefilipecosta]

Looks good in my tests 👍

[gziolo]

It uses a few more characters, but I think is a bit easier to read for those who aren't already familiar with the intent of the property utility function. This may be open to debate and I'm not strongly committed to keeping it this way.

Yes, I'm fine for not using for the time being. However, in the future, we should add one documentation page explaining basic FP patterns with an extensive list of external references to learn more. At the end of the day it's much easier to learn than async functions, generators and all those concepts. I guess the fact that lodash is semi-partially-almost-functional doesn't help here much.