Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[unbound-method] don't suppress errors thrown by base rule #1172

Closed
G-Rath opened this issue Jul 23, 2022 · 2 comments
Closed

[unbound-method] don't suppress errors thrown by base rule #1172

G-Rath opened this issue Jul 23, 2022 · 2 comments

Comments

@G-Rath
Copy link
Collaborator

G-Rath commented Jul 23, 2022

When I originally added our extension to unbound-method, I wanted to allow people to be able to enable it in shared ESLint configs themed around Jest rather than JavaScript or TypeScript i.e. so you could use such a config in projects regardless of if they were using TypeScript without needing to do anything extra.

We achieve this in two ways:

  1. We internally handle MODULE_NOT_FOUND errors when attempting to require the rule:

    const baseRule = (() => {
    try {
    // eslint-disable-next-line @typescript-eslint/no-require-imports
    const TSESLintPlugin = require('@typescript-eslint/eslint-plugin');
    return TSESLintPlugin.rules['unbound-method'] as TSESLint.RuleModule<
    MessageIds,
    Options
    >;
    } catch (e: unknown) {
    const error = e as { code: string };
    if (error.code === 'MODULE_NOT_FOUND') {
    return null;
    }
    throw error;
    }
    })();

  2. We swallow any errors thrown by the rule itself when attempting to create it:

    const tryCreateBaseRule = (
    context: Readonly<TSESLint.RuleContext<MessageIds, Options>>,
    ) => {
    try {
    return baseRule?.create(context);
    } catch {
    return null;
    }
    };

I think 2. is actually the wrong thing to do because notably it includes the "project property not specified" error that is thrown if you attempt to use the base rule without sufficient configuration for the parser services to be generated, without which the rule cannot work.

I don't think it would be a breaking change, but if we do a new major in the near-future we might as well include it at the same time - I will also be trying this on some of my projects at work to check I've not forgotten anything (tbh though I can't remember why I didn't think checking that @typescript-eslint/eslint-plugin was sufficient)

For reference, we do have a test that covers the current behaviour which'd effectively be inverted by this:

describe('when "project" is not set', () => {
const ruleTester = new ESLintUtils.RuleTester({
parser: '@typescript-eslint/parser',
parserOptions: {
sourceType: 'module',
tsconfigRootDir: rootPath,
},
});
ruleTester.run(
'unbound-method jest edition without "project" property',
requireRule(false),
{
valid: validTestCases.concat(invalidTestCases.map(({ code }) => code)),
invalid: [],
},
);
});

cc @SimenB @bradzacher for thoughts

@G-Rath G-Rath changed the title [unbound-method] don't suppress thrown errors when rule can be found [unbound-method] don't suppress errors thrown by base rule Jul 23, 2022
@github-actions
Copy link

🎉 This issue has been resolved in version 27.0.0-next.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link

🎉 This issue has been resolved in version 27.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants