refactor(searchbox): Remove unnecessary useEffect hook #6718
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
github-actions
bot
removed
the
github_actions:pull-request
|
Lighthouse Results
|
Unit Test Coverage Report
Unit Test Report
|
|
I have added the search function call in the event handlers as explained and I had to introduce the current selected facet parameter in the function.The function already had search term as an argument and I had to add the current selected facet because if I didn't, then the function would operate on a stale value of the selected facet. |
Interesting, I assume with an effect you didn't need to do so as it would already trigger a re-render, or? |
|
See using the effect is fine,but the way the effect is used right now especially with the use of |
Disabling an ESLint rule inline isn't a bad practice. The rule of hooks is "automatic" based on dependencies you're using, but it doesn't mean you need to pass those dependencies as triggers for the effect to re-run |
| @@ -135,14 +126,17 @@ export const WithSearchBox: FC<SearchBoxProps> = ({ onClose }) => { | |||
| router.push(`/search?q=${searchTerm}§ion=${selectedFacetName}`); | |||
| }; | |||
|
|
|||
| const changeFacet = (idx: string) => setSelectedFacet(Number(idx)); | |||
| const changeFacet = (idx: string) => { | |||
There was a problem hiding this comment.
Instead of casting the "idx" twice as a Number, why not cast it within the calls of changeFacet?
There was a problem hiding this comment.
It would, as it would explicitly tell that a facet must be a number. Otherwise I can pass "blah blah" as a param a a number conversion would be NaN; It enforces that whoever is calling the method is responsible for guaranteeing a number.
There was a problem hiding this comment.
onValueChange={changeFacet} changeFacet is being passed in the Tabs component.Its type must be (value: string) => void.
You can have a look at https://github.com/facebook/create-react-app/issues/6880 |
I'm not here to entertain you or discuss React's best practices. I won't enter on an argument with you on WHY what you're claiming is nonsensical. Simply linking to a random issue that has nothing to do with our conversation is baseless. The issue you've linked talks about "disabling" the eslint rule, which is indeed bad practice. What I'm referring is that it is a common practice to disable the rule at times when you believe it is misreporting, the eslint rule is known for always marking all the dependencies you're referring to be part as explicit dependencies for the hook lifecycle. But in many cases, you don't want to include all dependencies, nor need, intentionally. As changing the dependencies changes the behaviour of the Hook itself. |
There was a problem hiding this comment.
It is clear to me after a few exchange of messages that you don't know what you're doing; This change also is purely cosmetical as I can't really see any bug or behaviour difference between the current Production Environment and this PR;
In general PRs that change code just because of "stylistic" reasons or that don't actually fix a bug are not needed. I'm keen to be proven wrong and that actually this change improves the performance/behaviour of the search box.
For the meantime, I'm against the change.
I certainly know what I am doing and I was just trying to improve the code quality a bit. |
I understand you have good motivations, But this change, in my eyes, isn't a code quality improvement. It feels like a very subjective change, and if you have claimed that these changes actually "improve" the code, it'd be interesting for you to:
In my understanding, there isn't a real improvement here. That doesn't mean I'm right, and other folks might disagree with me. But for the meantime I just don't see clear benefits with this PR. I understand that this doesn't help you, but we, as a project, can't just accept every little PR that claims to fix or improve something. |
Yeah ok I see your point. |
|
Let's keep this PR open and wait for other reviewers to weigh in. I also apologize for the harsh tone in one of my previous messages; it was uncalled for. |
No worries for that. |
Description
Removes this unnecessary useEffect in the withSearchBox component.This useEffect can be removed because we have only 2 event handlers where this search function should be called(which is not a lot).
Also,
This effect properly calls the search api with
''as the search term and sets the initial search results so we dont need the above effect(the one that I removed) to run on mount with the empty search term and for subsequent changes in the search term and facet we can call the search function in the event handlers as explained above.Related Issues
Please refer #6717 to know more.
Check List
npm run formatto ensure the code follows the style guide.npm run testto check if all tests are passing.npx turbo buildto check if the website builds without errors.