refactor(searchbox): Remove unnecessary useEffect hook

[rahulkhattri0]

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

• [x] I have read the Contributing Guidelines and made commit messages that follow the guideline.
• [x] I have run npm run format to ensure the code follows the style guide.
• [x] I have run npm run test to check if all tests are passing.
• [x] I have run npx turbo build to check if the website builds without errors.
• [x] I've covered new added functionality with unit tests if necessary.

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
nodejs-org	✅ Ready (Inspect)	Visit Preview	May 11, 2024 10:18am

[github-actions[bot]]

Lighthouse Results URL	Performance	Accessibility	Best Practices	SEO	Report
/en	🟠 89	🟢 100	🟢 100	🟢 91	🔗
/en/about	🟢 99	🟢 100	🟢 100	🟢 91	🔗
/en/about/previous-releases	🟢 99	🟢 100	🟢 100	🟢 92	🔗
/en/download	🟢 100	🟢 100	🟢 100	🟢 91	🔗
/en/blog	🟢 100	🟢 100	🟢 96	🟢 92	🔗

[github-actions[bot]]

Unit Test Coverage Report

Lines	Statements	Branches	Functions
	90.04% (588/653)	76.08% (175/230)	92.18% (118/128)

Unit Test Report

Tests	Skipped	Failures	Errors	Time
128	0 :zzz:	0 :x:	0 :fire:	5.931s :stopwatch:

[rahulkhattri0]

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.

[ovflowd]

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?

[rahulkhattri0]

See using the effect is fine,but the way the effect is used right now especially with the use of // eslint-disable-next-line react-hooks/exhaustive-deps is not really a good practice.Also,useEffect must be avoided where it can be and here since we do not have a lot of event handlers to manage,useEffect can be avoided.

[ovflowd]

See using the effect is fine,but the way the effect is used right now especially with the use of // eslint-disable-next-line react-hooks/exhaustive-deps is not really a good practice.Also,useEffect must be avoided where it can be and here since we do not have a lot of event handlers to manage,useEffect can be avoided.

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

[rahulkhattri0]

See using the effect is fine,but the way the effect is used right now especially with the use of // eslint-disable-next-line react-hooks/exhaustive-deps is not really a good practice.Also,useEffect must be avoided where it can be and here since we do not have a lot of event handlers to manage,useEffect can be avoided.

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

You can have a look at https://github.com/facebook/create-react-app/issues/6880

[ovflowd]

See using the effect is fine,but the way the effect is used right now especially with the use of // eslint-disable-next-line react-hooks/exhaustive-deps is not really a good practice.Also,useEffect must be avoided where it can be and here since we do not have a lot of event handlers to manage,useEffect can be avoided.

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

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.

[rahulkhattri0]

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.

[ovflowd]

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:

• Explain what exactly is being improved and why current behaviour is not good, and how the current behaviour is impacting the code negatively
• Benchmarks/References or Screenshots/GIFs that prove visual changes of such benefits

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.

[rahulkhattri0]

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:

• Explain what exactly is being improved and why current behaviour is not good, and how the current behaviour is impacting the code negatively
• Benchmarks/References or Screenshots/GIFs that prove visual changes of such benefits

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.

[ovflowd]

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.

[rahulkhattri0]

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.