feat(unbound-method): create rule

[G-Rath]

This is an initial attempt at creating an "jest aware" version of @typescript-eslint/unbound-method that won't report on passing "unbound" methods to expect.

For now I've copied the rule and its tests (including fixtures) over verbatim and then kept modifications to a minimum so that it's easier to keep up to date with the base rule. (ideally would be nice if we could watch the base rule for changing some how 🤔).

It turns out that the logic is actually pretty straightforward, at least apparently to catch the common cases, but for the same reason I suspect I've missed an edge-case or two.

Aside from "what edge cases have I missed", my primary question right now is what should we do if we can't get the type service?

Personally, I'd really like to be able to enable this rule without having to worry about if the TypeScript service is available, as that'll make it a lot easier for sharable configs. However, the default behaviour is to throw saying that you need type information and its possible that people could be confused that the rule isn't do anything (however I think this'll be less likely for us, as the rule implies some exposure to the base rule and therefore TypeScript).

If we think the rule should error when type information is not available, I think it would be good to add an option to allow choosing between throwing an error or silently doing nothing, which could be used in shareable configs to safely enable the rule without having to worry.

I've created a test that hopefully captures the behaviour of the rule when running without the TypeScript service, but I could be completely wrong on that.

(I have just realised the rule actually imports ts for some enum values, so will have a think about this - we could drop that dependency easily by just inlining the enums and making it a type import 🤔)

@bradzacher would be good to have your input :)

Closes typescript-eslint/typescript-eslint#2951

[G-Rath]

I.e. it could be taught that expect(clazz.method).toBeTruthy() is safe, but expect(clazz.method).toThrow() is unsafe.

(will implement this now) implemented

[bradzacher]

if the only change you're making it to make this not report for expect calls, you can simplify this as an extension rule.

import TSESLintPlugin from '@typescript-eslint/eslint-plugin';
const baseRule = TSESLintPlugin.rules['unbound-method'];

// ...

export default util.createRule<Options, MessageIds>({
  ...baseRule,
  create(context) {
    const baseSelectors = baseRule.create(context);
    return {
      ...baseSelectors,
      CallExpression(node: TSESTree.CallExpression): void {
        inExpectCall = isJestExpectCall(node);
      },
      'CallExpression:exit'(node: TSESTree.CallExpression): void {
        if (inExpectCall && isJestExpectCall(node)) {
          inExpectCall = false;
        }
      },
      MemberExpression(node: TSESTree.MemberExpression): void {
        if (inExpectCall) {
          return;
        }
        baseRule.MemberExpression(node);
      },
    };
  },
})

Note that: this does create a dep on ts-eslint plugin.

You could make this safer and not add a direct dependency on the plugin with something like:

const baseRule = (() => {
  try {
    const TSESLintPlugin = require('@typescript-eslint/eslint-plugin');
    return TSESLintPlugin.rules['unbound-method'];
  } catch (e) {
    return null;
  }
})();

export default util.createRule<Options, MessageIds>({
  ...baseRule ?? { /* some defaults */ },
  create(context) {
    if (!baseRule) { return {} }
    return { ... };
  },
})

[G-Rath]

you can simplify this as an extension rule

That's a really good point! It's a lot cleaner to do it that way (thanks for the snippets btw).

I'd be happy with doing it that way, using the try/catch to avoid the hard dependency and make it an optional peer dependency to keep package managers happy.

[G-Rath]

ugh while I like the idea, re-writing this as an extension rule is proving hard to test because I can't re-require the actual rule to cover it's behaviour when the package "is not there" (aka throws an error):

describe('when @typescript-eslint/eslint-plugin is not available', () => {
  let rule = require('../unbound-method').default;

  beforeEach(() => {
    jest.resetModules();
    jest.mock('@typescript-eslint/eslint-plugin', () => {
      console.log('require');
      return () => {
        throw new Error();
      };
    });
    rule = require('../unbound-method').default;
  });

  new ESLintUtils.RuleTester({
    parser: '@typescript-eslint/parser',
    parserOptions: {
      sourceType: 'module',
      tsconfigRootDir: rootPath,
      project: './tsconfig.json',
    },
  }).run('unbound-method jest edition without type service', rule, {
    // valid: validTestCases.concat(invalidTestCases.map(({ code }) => code)),
    valid: invalidTestCases.map(({ code }) => code),
    invalid: [],
  });
});

This is what I've been hacking together, which fails because you can't have RuleTester in an it (as it creates its from the given test cases), but beforeEachruns for eachit` statement - so by the time the tester runs, the require has already happened :/

@SimenB what are you thoughts on what we should cover for this testing wise?

[G-Rath]

(in classic fashion, 5 minutes after my last comment, I glanced at some of the test code for eslint-plugin-eslint-config, which has similar tests, and realised how I should be able to test this 🤦)

[G-Rath]

I've left the original fixtures + tests in there for now, but I've gotten full cover with minimal hacking and without blowing out the run time of the test suite.

The only issue is that apparently there's improper teardown going on:

A worker process has failed to exit gracefully and has been force exited. This is likely caused by tests leaking due to improper teardown. Try running with --detectOpenHandles to find leaks.

I think maybe the TestRunner is staying alive longer than it should because the rule is erroring? Not sure why though 🙁

[G-Rath]

@SimenB would be keen to hear your thoughts on this - aside from the doc tweaks, this rule should be good to go.

I've also got a few ideas on some more type-based rules, one of which I've already got a semi-working implementation for (checking that .toThrow & co matchers are passed a function to be called).

[SimenB]

I'm a fan of type based rules 👍 I don't think they should go in recommended, but we can have a different rule set with type based ones

[G-Rath]

Awesome! I'll look to continue going 🎉

I'm updating the docs now, and then will address your changes - hopefully it should be out of draft sometime today.

[G-Rath]

I think we're good to go - @SimenB I'll leave this around for a bit in case you want to review the docs and changes; feel free to merge if you're happy and want to get it out, otherwise I'll look to do so sometime today or tomorrow :)

[SimenB]

something about needing to install the upstream plugin as well? Possibly in the table if we'll have othe rrules in the future that do not use type info

[G-Rath]

Possibly in the table if we'll have othe rrules in the future that do not use type info

You mean if we have other rules that don't extend base rules.

---

You're completely right - I don't expect us to make any more rules that extend base rules, only more rules that use type information; so I might leave the table as is for now and just add a sentence saying that unbound-method is an extension rule (as well as updating its own docs).