ts-migration/convert-is-utils-fns

[G-Rath]

Converts a couple of isX utility methods into their TS variants, so that it's easier to convert rules :)

[SimenB]

I wonder if we can make this (and the others) into a type guard. Sort of like what I've done in

eslint-plugin-jest/src/rules/lowercase-name.ts

Lines 7 to 51 in 6958f04

	type JestFunctionName = 'it' | 'test' | 'describe';
	interface JestFunctionIdentifier extends TSESTree.Identifier {
	name: JestFunctionName;
	}
	interface TestFunctionCallExpression extends TSESTree.CallExpression {
	callee: JestFunctionIdentifier;
	}
	type ArgumentLiteral = TSESTree.Literal | TSESTree.TemplateLiteral;
	interface FirstArgumentStringCallExpression extends TSESTree.CallExpression {
	arguments: [ArgumentLiteral];
	}
	type CallExpressionWithCorrectCalleeAndArguments = TestFunctionCallExpression &
	FirstArgumentStringCallExpression;
	const isItTestOrDescribeFunction = (
	node: TSESTree.CallExpression,
	): node is TestFunctionCallExpression => {
	const { callee } = node;
	if (callee.type !== AST_NODE_TYPES.Identifier) {
	return false;
	}
	const { name } = callee;
	return name === 'it' || name === 'test' || name === 'describe';
	};
	const hasStringAsFirstArgument = (
	node: TSESTree.CallExpression,
	): node is FirstArgumentStringCallExpression =>
	node.arguments &&
	node.arguments[0] &&
	(node.arguments[0].type === AST_NODE_TYPES.Literal ||
	node.arguments[0].type === AST_NODE_TYPES.TemplateLiteral);
	const isJestFunctionWithLiteralArg = (
	node: TSESTree.CallExpression,
	): node is CallExpressionWithCorrectCalleeAndArguments =>
	isItTestOrDescribeFunction(node) && hasStringAsFirstArgument(node);

It narrows down the types as we go

(might wanna move the ones I linked to in here as well?)

[G-Rath]

I'd advise against that - it's actually something I've recently been talking about with a few different people.

The problem is that type guards imply if something is or is not a given type, which will cause massive problems if you start putting business logic in your guards.

For example:

const isPositiveNumber = (n: unknown): n is number => typeof n === 'number' && n > 0;

const fn = (p: number | string): string => {
  if(isPositiveNumber(p)) {
    return p.toString();
  } //        ^ type: number

  return p.toUpperCase();
  //        ^ type: string | !number
}

fn(1);         // fine
fn('hello'); // also fine
fn(-1);       // error, no method toUpperCase on Number!

Another way (and maybe more accurate) to say is it that type guards don't just narrow, they also negate.

[G-Rath]

However in looking over your code, I think you might already know that, and has accounted for it :)

If you're happy with it, I'm happy with it 😂

[SimenB]

Yeah, they should definitely be logic free, just checking the type of things, not if they pass some constraint. But e.g. verifying the name of a function and the type of its arguments (or at least, say, the first n arguments) is really useful as we then don't have to use type casts to please the compiler. As long as we're careful to just do narrowing that types can verify, I think we're fine

[G-Rath]

Yeah totally - your code I think is perfect; it wouldn't work w/ the number example (that needs this), but it should be totally spot on for this.

The only thing I think is that the enums will need to be strings:

enum DescribeAlias {
  'describe' = 'describe',
  'fdescribe' = 'fdescribe',
  'xdescribe' = 'xdescribe',
}

Otherwise, we can't use them as both type values and as enums, since keyof DescribeAlias currently returns 0,1,2,etc :(

[SimenB]

That's fine 🙂 Not sure what the cleanest is there, but easy enough to change if a better alternative comes up at some point

[G-Rath]

I've pushed up an implementation, which I am actually really happy w/.

Despite how it might first appear, I think it shuffles around the current learning curve rather than adding to it, while making it super quick to implement scoping on rules to target specific jest functions :D

Let me know what you think - I've applied it to lowercase-name as a showcase, but can cherry-pick that into another PR if you'd like.

[SimenB]

Woo, thanks! I left a comment that I'd love to see resolved, but we can also do that as a follow-up

[SimenB]

Hmm, CI also fails since these helpers are not used... Can add an istanbul ignore comment above until we use them

[SimenB]

doesn't (isTestCase(node) || isDescribe(node)) cover this line?

[G-Rath]

I was going to ask you about that :)

Previously the check was just for these three functions, and the description & tests for the rule only focus on those three:

Enforce it, test and describe to have descriptions that begin with a lowercase letter. This provides more readable test failures.

I think it should be fine to remove that line, but wanted to implement the original behaviour + ask first, in case that was part of the point of that rule that the community requested (or something)

[SimenB]

ah right, since we'd now get xtest, ftest etc? I think it's fine to detect all - they're just modifiers on the same functions

[SimenB]

We can do that in a follow-up, though

[G-Rath]

btw I've had to remove the commitlint hook from package.json, as it just refuses to work properly.

Could you confirm for me if it's working on your end, and what OS you're using?

I'm on Win10 w/ WSL - I've tried removing & reinstalling from both cmd & wsl, plus updating husky, replacing just the commitmsg hook w/ the latest version from husky while still usingv1, etc all to no success :(

[G-Rath]

I've started converting the expectCase methods too.

Let me know if you'd like them in a separate PR; otherwise I'll push them up to this one soon :)

I'm applying the typeguard patten where possible, renaming such methods to "is" and leaving an exported const alias in place of the old version w/ a @deprecated link.