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

💅 Lost ability to lint React hooks from specific namespace #1931

Closed
1 task done
catherinelasersohn opened this issue Feb 27, 2024 · 7 comments · Fixed by #1973
Closed
1 task done

💅 Lost ability to lint React hooks from specific namespace #1931

catherinelasersohn opened this issue Feb 27, 2024 · 7 comments · Fixed by #1973
Labels
A-Analyzer Area: analyzer S-Bug-confirmed Status: report has been confirmed as a valid bug

Comments

@catherinelasersohn
Copy link

Environment information

CLI:
  Version:                      1.3.3
  Color support:                true

Platform:
  CPU Architecture:             x86_64
  OS:                           macos

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         "xterm-256color"
  JS_RUNTIME_VERSION:           "v20.9.0"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "npm/10.2.4"

Biome Configuration:
  Status:                       Loaded successfully
  Formatter disabled:           false
  Linter disabled:              false
  Organize imports disabled:    true
  VCS disabled:                 true

Workspace:
  Open Documents:               0

Rule name

useExhaustiveDependencies

Playground link

https://biomejs.dev/playground/?code=aQBtAHAAbwByAHQAIAAqACAAYQBzACAAUgBlAGEAYwB0AFIAZQBlAHgAcABvAHIAdAAgAGYAcgBvAG0AIAAiAHIAZQBhAGMAdABfAHIAZQBlAHgAcABvAHIAdAAiADsACgBpAG0AcABvAHIAdAAgACoAIABhAHMAIABSAGUAYQBjAHQAIABmAHIAbwBtACAAIgByAGUAYQBjAHQAIgA7AAoACgAKAC8ALwAgAFQAaABpAHMAIABpAHMAIABsAGkAbgB0AGUAZAAgAGEAcwAgAGUAeABwAGUAYwB0AGUAZAAKAGYAdQBuAGMAdABpAG8AbgAgAGMAbwBtAHAAbwBuAGUAbgB0ACgAKQAgAHsACgAgACAAIAAgAGwAZQB0ACAAYQAgAD0AIAAxADsACgAgACAAIAAgAFIAZQBhAGMAdABSAGUAZQB4AHAAbwByAHQALgB1AHMAZQBFAGYAZgBlAGMAdAAoACgAKQAgAD0APgAgAHsACgAgACAAIAAgACAAIAAgACAAYwBvAG4AcwBvAGwAZQAuAGwAbwBnACgAYQApADsACgAgACAAIAAgAH0ALAAgAFsAXQApADsACgAgACAAIAAgAFIAZQBhAGMAdAAuAHUAcwBlAEUAZgBmAGUAYwB0ACgAKAApACAAPQA%2BACAAewAKACAAIAAgACAAIAAgACAAIABjAG8AbgBzAG8AbABlAC4AbABvAGcAKABhACkAOwAKACAAIAAgACAAfQAsACAAWwBdACkAOwAKAH0A

Expected result

If react_reexport is a file that exports useEffect from React, I'd expect the linter to also catch the useExhaustiveDependencies error for ReactReexport.useEffect (as it does for React.useEffect when we import from react directly).

The eslint equivalent exhaustive-deps has the ability to specify additional namespaces from which we can import and lint hooks

Code of Conduct

  • I agree to follow Biome's Code of Conduct
@arendjr
Copy link
Contributor

arendjr commented Feb 28, 2024

Is this still a problem if you configure useEffect in the rule options? https://biomejs.dev/linter/rules/use-exhaustive-dependencies/#options

It doesn’t allow you to specify a namespace, but I think it might allow linting it from any source.

@catherinelasersohn
Copy link
Author

Yes it's still a problem even when configuring the options. Adding useEffect as a hook here still does not trigger linting on useEffect imports from a custom path

@arendjr arendjr added S-Bug-confirmed Status: report has been confirmed as a valid bug A-Analyzer Area: analyzer labels Feb 29, 2024
@arendjr
Copy link
Contributor

arendjr commented Feb 29, 2024

Got it. If we changed that behavior, would it solve the problem for you? Or do you think adding configurable namespaces would still be better?

@catherinelasersohn
Copy link
Author

Changing that behavior would be great, thanks! The most important thing is that hooks including the custom namespace are also linted (in addition to hooks from custom paths). For example, both uses of useEffect are correctly linted here:

import * as React from "react";
import {useEffect} from "react";

useEffect(() => {}, []);
React.useEffect(() => {}, []);

However, when we use the custom namespace, neither use of useEffect is linted here:

import * as ReactReexport from "react_reexport";
import {useEffect} from "react_reexport";

useEffect(() => {}, []);
ReactReexport.useEffect(() => {}, []);

To solve the problem the second two uses of useEffect would need to be linted in addition to the first two (which are already working)

@catherinelasersohn
Copy link
Author

@arendjr Thank you for the help and fix for this!

@arendjr
Copy link
Contributor

arendjr commented Mar 6, 2024

@catherinelasersohn please be aware there is still a downside to the current implementation, which is that hooks with stable return values aren’t being recognized as stable either yet when they’re being imported from other places. For instance normally, you don’t need to specify setters from useState() in your dependency arrays, because they are always stable. But while the fix I implemented allows you to specify that custom useEffect() hooks should be validated, it doesn’t yet allow you to configure custom useState() hooks as having stable return values. For that, I’m picking up #1128

@catherinelasersohn
Copy link
Author

@arendjr Thanks for clarifying and for the stable hook fix as well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Analyzer Area: analyzer S-Bug-confirmed Status: report has been confirmed as a valid bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants