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

Reenable eslint rule @typescript-eslint/unbound-method #9082

Closed
Tracked by #9501 ...
markfields opened this issue Feb 10, 2022 · 12 comments · Fixed by #9696
Closed
Tracked by #9501 ...

Reenable eslint rule @typescript-eslint/unbound-method #9082

markfields opened this issue Feb 10, 2022 · 12 comments · Fixed by #9696
Assignees
Labels
area: repo Repo related work bug Something isn't working
Milestone

Comments

@markfields
Copy link
Member

markfields commented Feb 10, 2022

@tylerbutler - This rule is pretty important if I understand it correctly, why did you disable it?

Originally posted by @markfields in #8547 (comment)

More info on the rule is here: https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/unbound-method.md

Things will break badly if we start breaking it, as far as I can tell.

@ghost ghost added the triage label Feb 10, 2022
@markfields markfields added the area: repo Repo related work label Feb 10, 2022
@tylerbutler
Copy link
Member

According to the comment in the minimal config: "This rule has false positives in many of our test projects."

@tylerbutler
Copy link
Member

tylerbutler commented Feb 10, 2022

The rule is only completely disabled in the eslint7 config, which is no longer used by any package in the repo since the upgrade to eslint 8.

The default config is now "minimal," which does disable the rule, but only for test files, where there are lots of false positives.

We could consider using the jest version of the rule, though, as the docs suggest.

@markfields
Copy link
Member Author

markfields commented Feb 10, 2022

Hmmm that's interesting. As-is I can write a violation of the rule but not see an error. On the other hand, when I added it directly to the package's .eslintrc.js it did get caught.

@markfields
Copy link
Member Author

I see it's disabled for test files, but... where is it enabled to begin with? Are we missing something in the extends list?

@markfields
Copy link
Member Author

the eslint7 config, which is no longer used by any package in the repo

Can we delete it from main?

@markfields
Copy link
Member Author

@ChumpChief - Assigning this to you since you're looking into restoring this and bunch of other rules to the common config.

@ChumpChief
Copy link
Contributor

Reactivated #9093 to track getting the bulk of the rules back on (currently guessing I'll extend eslint7 in minimal). Even with that, the unbound-method rule is disabled in eslint7 so this should be considered separately.

@tylerbutler
Copy link
Member

@ChumpChief @markfields I was reminded of why this got disabled originally... In a lot of projects, the rule throws an exception:

TypeError: Cannot read property '0' of undefined
Occurred while linting /workspaces/FluidFramework/common/lib/container-definitions/src/browserPackage.ts:58
Rule: "@typescript-eslint/unbound-method"
    at checkMethod (/workspaces/FluidFramework/common/lib/container-definitions/node_modules/@typescript-eslint/eslint-plugin/dist/rules/unbound-method.js:220:47)
    at checkMethodAndReport (/workspaces/FluidFramework/common/lib/container-definitions/node_modules/@typescript-eslint/eslint-plugin/dist/rules/unbound-method.js:155:53)
    at MemberExpression (/workspaces/FluidFramework/common/lib/container-definitions/node_modules/@typescript-eslint/eslint-plugin/dist/rules/unbound-method.js:177:17)
    at ruleErrorHandler (/workspaces/FluidFramework/common/lib/container-definitions/node_modules/eslint/lib/linter/linter.js:1079:28)
    at /workspaces/FluidFramework/common/lib/container-definitions/node_modules/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (/workspaces/FluidFramework/common/lib/container-definitions/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (/workspaces/FluidFramework/common/lib/container-definitions/node_modules/eslint/lib/linter/node-event-generator.js:297:26)
    at NodeEventGenerator.applySelectors (/workspaces/FluidFramework/common/lib/container-definitions/node_modules/eslint/lib/linter/node-event-generator.js:326:22)
    at NodeEventGenerator.enterNode (/workspaces/FluidFramework/common/lib/container-definitions/node_modules/eslint/lib/linter/node-event-generator.js:340:14)

I tried upgrading @typescript-eslint/eslint-plugin but it still repros. I'll start looking through their open issues...

@markfields
Copy link
Member Author

That is a real bummer

@ChumpChief
Copy link
Contributor

This just bit me today, I'm gonna prioritize it 😆

@markfields
Copy link
Member Author

Thank you @ChumpChief ! What about the error Tyler mentioned above?

@ChumpChief
Copy link
Contributor

I didn't hit it personally, so not sure exactly.

@ChumpChief ChumpChief removed the triage label Apr 5, 2022
@ChumpChief ChumpChief moved this to Done in Fluid DevX Apr 5, 2022
@ChumpChief ChumpChief added the bug Something isn't working label Apr 5, 2022
@ChumpChief ChumpChief added this to the April 2022 milestone Apr 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: repo Repo related work bug Something isn't working
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants