Let's go through a couple of observations on why original code doesn't work as intended
Mispurposed filtering
In original SearchHandler you have this piece
(data => {
Object.keys(data).filter(element => {
if (queryString === "") {
setResult(result.push(data[element].studentName));
} else if (data[element].studentName.toLowerCase().includes(queryString.toLowerCase())) {
setResult(result.push(data[element].studentName));
}
setResult(result.join("<br/>"));
})
});
So there Object.keys(data) gets all the keys of data and then filter runs through every single key and returns nothing.
filter callback is supposed to return a boolean so that filter method can decide which item of the array to store and which not to.
And then ideally you are supposed to do something with the returned filtered array ideally which is not happening here.
In the end of this post I will suggest what exactly.
Setting state with the wrong result type
Next let's focus on this portion
setResult(result.push(data[element].studentName));
Here you have 2 things happening:
result.push(data[element].studentName) that returns the number of elements of a new array after push. Let's for the sake of example say it's 5- And then you call
setResult with that 5 i.e. setResult(5)
I think this is not what you intended to persist in the result state. But it gets even deeper.
Note on setState scheduling
Let's pivot a little into how react schedules setState calls to be able lay ground for our further explanations
For a more in-depth answer on that please check the details here https://stackoverflow.com/a/48610973/2998898
But in essence, at the time of this writing, React (v17) currently batches multiple sibling setState calls happening in the same asynchronous callback only if they are called directly in event handler body (like onClick, onChange, ...etc) or directly in the body of hooks (like useEffect).
To visualize the concept have a look at the console in this example
function App() {
const renderNumberRef = React.useRef(1);
React.useEffect(() => {
renderNumberRef.current += 1;
});
const [result, setResult] = React.useState([]);
function searchHandler() {
Promise.resolve().then(() => {
console.log("enter searchHandler() promise callback");
console.log("setResult() with student1");
setResult([...result, "student1"]);
console.log("setResult() with student2");
setResult([...result, "student2"]);
console.log("setResult() with student3");
setResult([...result, "student3"]);
console.log("exit searchHandler() promise callback");
});
}
console.log(
"Active render number: ",
renderNumberRef.current,
"state.result: ",
result
);
return (
<div>
<div>
<button onClick={searchHandler}>
<div>Search Icon</div>
</button>
</div>
</div>
);
}
ReactDOM.render(<App />, document.querySelector('#root'))
<script src="https://cdnjs.cloudflare.com/ajax/libs/react/16.8.3/umd/react.production.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/react-dom/16.8.3/umd/react-dom.production.min.js"></script>
<div id="root"></div>
In your case setResult() is called inside of searchHandler which is indeed an event handler, BUT it's not in its direct body, it's in fact in a nested fetch().then() promise callback.
That's why instead of batching react will immediately rerender the component on every setResult() call, which will save the value 5 into state over and over and over again until the end of searchHandler promise callback
Fixed reference
Now you might ask why 5 though?
Aren't we supposed to now break on setResult(5.push()) on the very next setResult() call?
We would if it weren't for to the fact that we use a closure reference to the same result variable inside of searchHandler so what you actually do is you keep setResult()-ing the same initial empty array over and over again until the searchHandler function is finished executing.
And every time rerender happens during execution of searchHandler we keep reassigning the same value 5 into state
The Error
And the very next time you click search button again you now take result which now contains 5
And the logic immediately breaks because 5.push() is a Type Error
Potential solution
So as you can see there are many things going wrong in the same place at the same time.
So first you need to make sure you use array filtering with its' primary purpose of returning a filtered array.
You take that array and you save it into the state once after filtering is finished.
Calling setResult multiple times within a filter or while your event is still going is suboptimal and doesn't serve your purpose.
You should always try to mitigate amount of unnecessary rerenders.
The simple mental model that should help is this:
- fetch some data
- transform/filter the data
- set the final result into state
- use the result from state to render something on the screen
So I created a rewritten and mocked version of your initial code with explanations within inline comments.
const mockFetch = () =>
Promise.resolve({
json: () =>
new Promise((resolve) => setTimeout(() => resolve({
element1: { studentName: "student1" },
element2: { studentName: "student2" },
element3: { studentName: "student3" }
// imitating 250ms delay with mocked fetch here
}), 250))
});
function App() {
const [result, setResult] = React.useState([]);
const [queryString, setQueryString] = React.useState("");
function searchHandler(capturedQueryString) {
mockFetch("{url that i defined in real code}")
.then((response) => response.json())
.then((data) => {
// for each search you do not need to run setResult on
// each iteration like in your initial code above in filter method
// you just need to calculate once your whole state per request and then
// set the result in state
if (capturedQueryString === "") {
// pay attention here we don't setResult(element.push())
// because array.push() returns an integer that
// signifies the length of the new array
// so in your code above you were persisting the number itself
// and then calling push() method of a number which fails
// so instead we persist the array itself
setResult(
Object.keys(data).map((element) => data[element].studentName)
);
} else {
setResult(
Object.keys(data)
.filter((element) => {
return data[element].studentName
.toLowerCase()
.includes(capturedQueryString.toLowerCase());
})
.map((filteredElement) => data[filteredElement].studentName)
);
}
});
}
return (
<div>
<div>
<input
type="text"
id="search-bar"
placeholder="Search..."
onChange={(event) => setQueryString(event.target.value)}
/>
{/* passing in captured queryString
// so that when clicked within searchHandler
// we always operate with the same queryString
// regardless if queryString has changed in state or not
*/}
<button onClick={() => searchHandler(queryString)}>
<div>Search Icon</div>
</button>
</div>
<output>
Result:
<br />
<br />
{/* In order to join your array with <br /> tag you need to inner html it */}
{/* React provides that possibility via dangerouslySetInnerHTML api */}
{/* BUT since your student name can be hijacked by attacker given that your api is also not sanitized */}
{/* the best practice is to sanitize the joined string with something like sanitize-html or DOMPurify package */}
<span dangerouslySetInnerHTML={{__html: result.join('<br />')}} />
<br />
<br />
<br />
<br />
Query String:
<br />
<br />
{queryString}
<br />
<br />
</output>
</div>
);
}
ReactDOM.render(<App />, document.getElementById('root'))
<script src="https://cdnjs.cloudflare.com/ajax/libs/react/16.8.3/umd/react.production.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/react-dom/16.8.3/umd/react-dom.production.min.js"></script>
<div id="root"></div>
P.S. Of course this is not a finished version of the functionality, since it doesn't cover plethora of edge cases like (what happens if you click 3 times and 2nd request resolves after 3rd, or what if one of the requests gets rejected etc etc) but this should be a fine start and should hopefully unblock you.
) in each filter iteration? – Karen Grigoryan Oct 20 '21 at 20:12