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

False positive in consistent-function-scoping when using React.useEffect #1685

Closed
kachkaev opened this issue Jan 9, 2022 · 4 comments · Fixed by #1691
Closed

False positive in consistent-function-scoping when using React.useEffect #1685

kachkaev opened this issue Jan 9, 2022 · 4 comments · Fixed by #1691
Assignees
Labels

Comments

@kachkaev
Copy link
Contributor

kachkaev commented Jan 9, 2022

Since #588, consistent-function-scoping is not triggered for the following code:

import { useEffect } from "react";

useEffect(() => {
  function foo() {}
}, []);

This makes sense – see #392. However, when React is imported as a namespace, the same code is no longer valid:

import * as React from "react";

React.useEffect(() => {
  function foo() {} // Move function 'foo' to the outer scope. eslint(unicorn/consistent-function-scoping)
}, []);

Reporting React as a namespace is OK. I prefer this method because it produces smaller git diffs and prevents no-unused-var when refactoring.

Happy to submit a PR with the fix around next weekend. If anyone wants to create one earlier, please feel free to! 🙌

Relevant condition and its test:

const isReactHook = scope =>
scope.block
&& scope.block.parent
&& scope.block.parent.callee
&& scope.block.parent.callee.type === 'Identifier'
&& reactHooks.has(scope.block.parent.callee.name);

// React Hooks
outdent`
useEffect(() => {
function foo() {}
}, [])
`,

@kachkaev kachkaev added the bug label Jan 9, 2022
@fisker fisker self-assigned this Jan 10, 2022
@fisker
Copy link
Collaborator

fisker commented Jan 11, 2022

Happy to submit a PR with the fix around next weekend.

I missed this, happy to let you send the PR, you can use

function isNodeMatches(node, nameOrPaths) {

@kachkaev
Copy link
Contributor Author

kachkaev commented Jan 11, 2022

Thanks @fisker! Let me give it a go real quick (unless you have already started) 🙂

@fisker
Copy link
Collaborator

fisker commented Jan 11, 2022

I didn't.

@kachkaev
Copy link
Contributor Author

kachkaev commented Jan 11, 2022

The fix: #1691 👀

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

Successfully merging a pull request may close this issue.

2 participants