Skip to content

Conversation

@possum-enjoyer
Copy link
Contributor

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • Test
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • I have added a convincing reason for adding this feature, if necessary

Other information

@vercel
Copy link

vercel bot commented Nov 15, 2025

@possum-enjoyer is attempting to deploy a commit to the Rel1cx's projects Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Nov 16, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
eslint-react Ready Ready Preview Comment Nov 26, 2025 11:54am

@possum-enjoyer
Copy link
Contributor Author

I found a problem: currently if the useCallback is double useless (i.e. no dependencies and only inside one useEffect) the rule will show up twice. Should there be a priority?

@Rel1cx
Copy link
Owner

Rel1cx commented Nov 23, 2025

I found a problem: currently if the useCallback is double useless (i.e. no dependencies and only inside one useEffect) the rule will show up twice. Should there be a priority?

If these two error messages are granular enough to specify a particular "useless" situation, displaying two is reasonable; otherwise, displaying only one is appropriate.

@possum-enjoyer
Copy link
Contributor Author

I found a problem: currently if the useCallback is double useless (i.e. no dependencies and only inside one useEffect) the rule will show up twice. Should there be a priority?

If these two error messages are granular enough to specify a particular "useless" situation, displaying two is reasonable; otherwise, displaying only one is appropriate.

I would say they are granular enough with a small asterisks that is:
Both error messages showing up can be a good thing for a developer to see that the function be moved outside the component or inside the useEffect. Moving it inside the useEffect may be good for scoping the function with the call but it might be a little bit overwhelming having two error messages at the same time.

@Rel1cx
Copy link
Owner

Rel1cx commented Nov 23, 2025

I found a problem: currently if the useCallback is double useless (i.e. no dependencies and only inside one useEffect) the rule will show up twice. Should there be a priority?

If these two error messages are granular enough to specify a particular "useless" situation, displaying two is reasonable; otherwise, displaying only one is appropriate.

I would say they are granular enough with a small asterisks that is: Both error messages showing up can be a good thing for a developer to see that the function be moved outside the component or inside the useEffect. Moving it inside the useEffect may be good for scoping the function with the call but it might be a little bit overwhelming having two error messages at the same time.

I found a problem: currently if the useCallback is double useless (i.e. no dependencies and only inside one useEffect) the rule will show up twice. Should there be a priority?

If these two error messages are granular enough to specify a particular "useless" situation, displaying two is reasonable; otherwise, displaying only one is appropriate.

I would say they are granular enough with a small asterisks that is: Both error messages showing up can be a good thing for a developer to see that the function be moved outside the component or inside the useEffect. Moving it inside the useEffect may be good for scoping the function with the call but it might be a little bit overwhelming having two error messages at the same time.

Showing only one message means we're hiding part of the explanation or suggestion for this bad pattern; from this perspective, I prefer to show all error messages.

@Rel1cx
Copy link
Owner

Rel1cx commented Nov 23, 2025

but it might be a little bit overwhelming having two error messages at the same time.

We also have a solution for this. If users report two error messages too verbose, we can define only one error message and dynamically construct it to best reflect the actual situation without information missing. Similar to the practice in a rule:

suggestion: component.parent.type === T.Property
? "Move it to the top level or pass it as a prop."
: "Move it to the top level.",

This approach is more suitable for complex rules.

@Rel1cx
Copy link
Owner

Rel1cx commented Nov 25, 2025

I think we also need to update the documentation for these two rules to include the newly introduced patterns.

@possum-enjoyer
Copy link
Contributor Author

I think we also need to update the documentation for these two rules to include the newly introduced patterns.

Yes that's true. Sorry I forgot about this when starting this "addition" it looked smaller and more like an edge case for the rules.
I will add that and look into the construction of a unified error message :)

fix both rules appearing at the same time. Now the original error message takes precedence
@possum-enjoyer
Copy link
Contributor Author

but it might be a little bit overwhelming having two error messages at the same time.

We also have a solution for this. If users report two error messages too verbose, we can define only one error message and dynamically construct it to best reflect the actual situation without information missing. Similar to the practice in a rule:

suggestion: component.parent.type === T.Property
? "Move it to the top level or pass it as a prop."
: "Move it to the top level.",

This approach is more suitable for complex rules.

Ok i fixed this but currently i did a different approach where check if the error is reportable, return the report object and report only at specific points where the original error message would not report. So the original error message takes precedence.
I thought this might be clearer than constructing the error message dynamically because both error messages are very different in what they want to convey.

Also the documentation is now updated :)

Copy link
Owner

@Rel1cx Rel1cx left a comment

Choose a reason for hiding this comment

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

LGTM.

@Rel1cx Rel1cx merged commit 0b72027 into Rel1cx:main Nov 26, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants