ts-migration/prefer-called-with

[G-Rath]

@yatki I've stepped on your toes a little bit here - sorry about that 😬

[G-Rath]

Ping me before you merge this - I'm been rewriting the last the rules, resulting in some super new generic-ish guards that might supersed this PR.

[yatki]

JestExpectCallWithNotParent looks like a Call without parent. Don't you think ExpectNotCall is a better naming @G-Rath :) Also we could avoid conflicts :D

[G-Rath]

Maybe? iirc I don't think b/c the parent is the Not.

I've actually spent the whole day pretty much writing a completely typesafe implementation of the guards that are also completely compatible w/ identifiers, literals and template literals, and am now converting the remaining rules right now.

I hope to push them up pretty soon, but they're still somewhat messy.

There was always going to be naming conflicts unfortunately.

Just to give you a little taste of the new guards in action:

const fn2 = (node: TSESTree.Node) => {
  if (
    isExpectCallWithSpecificParent(node, 'not') &&
    isExpectCallWithSpecificGrandParent(node, 'toBe')
  ) {
    if (getAccessorExpressionValue(node.parent.parent.property) === 'not') {
        // ^^ TS error - this will never be true
    }
    
    if (getAccessorExpressionValue(node.parent.property) === 'resolves') {
    }

    if (getAccessorExpressionValue(node.parent.parent.property) === 'toBe') {
    }
  }
};

[yatki]

oh looks nice.. Ok, no worries. Keep doing what you think is the best. As long as @SimenB is ok with these changes, later I can refactor my PR accordingly. 🖖

[G-Rath]

No problem :)

Your PR was a huge help, as it helped confirm a lot of things I wanted to confirm before trying to rewrite all the guards.

Would it be ok if I pulled your PR into mine? Just in case I need to to hit 100% coverage & ensure the guards are all correct.

I'm definitely going to want as many eyes as I can on it, as there's definitely redundancy, so I'll make sure to ping you when I push up a draft PR (which I might be doing very soon).

[SimenB]

I don't really have an opinion either way. In general I'd like to avoid the parent terminology since I find it weird. But I think we should use it for now since it represents the AST nodes, and keeping the names correct makes it easier to reason about. Once we've ported all rules and we've got strong type safety in the utils, I'd like to revisit #73 (comment). So you can some kind of custom structure back and instead of accessing .parent (or .parent.parent in case of not) you just do .matcher and check a boolean .isNot or something.

---

Anyways, rambling way of saying "these names are fine for now, I'd like to revisit after migration is complete to see if we can simplify" 🙂

[G-Rath]

I completely agree, and just you wait until you see some of the new names :D

Tbh, while the names seem to go against what we've taught as programmers, in AST land they're actually really nice - things like isExpectCallWithJestNamespace (here's a structural screenshot which contains about half the types & guards: http://puu.sh/DWjAi/e0a141eab4.png) that are actually really nicely verbose once you get use to using them in the land of AST where there's lots of annoying layers.

I do totally agree w/ the parent thing - it's super annoying, b/c I've not been able to think up of a better name that isn't less accurate or technically incorrect in some way :/

[SimenB]

For sure, I think we should keep the verbose names as they more directly map to the underlying AST nodes we operate on. But down the line we can create more human readable abstractions on top of it to easily make assertions about different matchers (while still keeping the verbose versions as the implementation)

[G-Rath]

I think that's exactly the right way to go about it, and what I've been working towards.

It's already how we're doing accessors now:

type SpecificAccessorExpression<Specifics extends string> =
  | SpecificStringNode<Specifics>
  | SpecificIdentifier<Specifics>;

type AccessorNode = SpecificAccessorExpression<string>;

const isSupportedAccessor = (node: TSESTree.Node): node is AccessorNode =>
  node.type === AST_NODE_TYPES.Identifier || isStringNode(node);

export const getAccessorExpressionValue = <S extends string = string>(
  accessor: SpecificAccessorExpression<S>,
): S =>
  accessor.type === AST_NODE_TYPES.Identifier
    ? accessor.name
    : getSpecificStringValue(accessor);

[SimenB]

could possibly have a version with this that receives a JestExpectCallWithParent which just checks the parent? in this rule, we call isExpectCallWithParent twice. Probably not a big issue as the checks are pretty cheap

[SimenB]

LGTM. Can hold off on merge if you want @G-Rath?

[G-Rath]

@SimenB yes please - I'm going to make a WIP PR very soon I think, so that you can start picking it to bits while I'm still working on it :P

[G-Rath]

btw @yatki something you could do that'd be a huge help would be to figure out how to get the getToken<Before/After> & get<First/Last>Token methods on SourceCode to return null, so that we can test them.

Right now we just have istanbul ignore the if condition, and throw a unique error:

               throw new Error(
                `Unexpected null when attempting to fix ${context.getFilename()} - please file a github issue at https://github.com/jest-community/eslint-plugin-jest`,
              );

Finding a way to reproduce that would be fantastic, cause it's got me totally stumped. Check out no-test-callback if you're up for the challenge ;)

[SimenB]

Do we wanna keep this open? I know #363 will close it, but maybe we can land this separately as well as we've been landing other parts of #363 already?

[G-Rath]

@SimenB yeah this should be closed - we need parseExpectCall to land it 😬

[SimenB]

🎉 This issue has been resolved in version 22.15.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀