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

[ESLint] Extend isHook to recognize those under PascalCase's namespace #18722

Merged
merged 5 commits into from
Apr 24, 2020

Conversation

cyan33
Copy link
Contributor

@cyan33 cyan33 commented Apr 23, 2020

Summary

Context and discussion: https://fburl.com/ppb00y2s

TLDR: A few teams have the convention to put hooks under a namespace and the current implementation doesn't catch hooks that are written this way.

// is hook
useSomething();

// is not hook => doesn't recognize
Foo.useSomething();

This prevented us from catching the true positives and started to cause issues, like T65929958. We want to extend the definition of isHook so that we can safely catch those that are used:

  • on top level / not in React component or other hooks
  • called in class component
  • being conditionally called

We intentionally don't include the namespace that are named in "camelCase" because there're a lot of false positive, eg:

jest.useFakeTimers

This change requires us to move a few test cases that are used to be regression tests and were valid to invalid.

Test Plan

# under react
> yarn test ESLintRulesOfHooks-test.js

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 88c614c:

Sandbox Source
optimistic-ramanujan-zt3pi Configuration

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 23, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 7e3671a:

Sandbox Source
distracted-resonance-npvz4 Configuration

@sizebot
Copy link

sizebot commented Apr 23, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 7e3671a

@sizebot
Copy link

sizebot commented Apr 23, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 7e3671a

{
code: `
// Extending the isHook check to [A-Z].*\.use
// to not let it be called on top level
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this comment. I think you can just drop this example now because it is also a false positive. Or you could come up with another way to avoid flagging it (e.g. lowercase history or rename Hook-like function at destructuring) and move it back to valid.

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

Successfully merging this pull request may close these issues.

4 participants