6

I have a simple dictionary app I'm making that returns search results. A search is queried on every onChange of the search field using a lokiJS in-memory database for the dictionary, so the results come very quick.

It is important for me to optimize the rendering of the results, so that the 50 or so filtered search results keep flowing as the user types and the queries happen on each key stroke.

Here's what I've been doing so far: (this works but is it the best/fastest way?)

Each (sync) query of the database returns an array of objects, which I then map with something like this: 

queryDB(query) {
  const results = queryLokiJsDB(query);
  const resultsMapped = results.map((mpd) =>
        <dl key={mpd["$loki"]} onClick={() => this.clickFunction(mpd.p, mpd.f)}>
          <dt>{mpd.p} - {mpd.f}</dt> <dd>{mpd.e} <em>{mpd.c}</em></dd>
        </dl>);
  this.setState({ results: ( 
    <div>
      {resultsMapped}
    </div>
  )});
}

Then, once I have the results mapped like that, I add the mapped components to the state, and then the screen gets rendered with the new results.

render() {
  return (
    <div>
      <SearchBarStuff />
      <div className="results-container">
          {this.state.results}
      </div>
    </div>
  );
}

I made this when I was just learning React and I understand that people consider it really bad practice to store components in the state.

I was wondering what the best practice in terms of performance optimization would be (and code cleanness). Should I store the results array in state then render then map them into components in the render function? Would that cause any performance hits?

Another answer said that "A function in the render method will be created each render which is a slight performance hit."

And why is it so bad to store components in state? Is it just for code clarity? Keeping the state small? I appreciate your help and patience.

Adam D
  • 1,962
  • 2
  • 21
  • 37
  • 2
    If I were to guess, I think it's bad practice to store components in state because React has to compare the previous state with the new state to see if it needs to re-render, and if it has to crawl through such a large array of components instead of an array of objects, you're making it do more work. It's definitely best practice to store the array of objects in state and then handle the mapping and rendering of them in the component. – larz Aug 14 '18 at 19:49
  • @larz But the React docs say that it compares the state in a shallow way, meaning that for objects it would check only if the references changed. In the example of the OP, each time `setState({results})` is called, React would see rightaway that `results` has changed because the new component instance would be a different object reference. Therefore I would say that performance in this case is not compromised. – tonix Jul 12 '19 at 14:30

1 Answers1

1

I'm not quite sure but I am thinking the same problem as @larz explained in comments. Also, as you mentioned the state will be clearer. So here what would I do if I were you.

First, set your state just with the results:

queryDB(query) {
    const results = queryLokiJsDB(query);
    this.setState({ results });
}

Then, map through the results but instead of creating your JSX immediately I would use a separate component. Pass it mpd (the element) and onClick function with its reference.

render() {
    return (
        <div>
            <SearchBarStuff />
            <div className="results-container">
                {this.state.results.map( mpd => (
                     <Item key={mpd["$loki"]} mpd={mpd} onClick={this.clickFunction} />
                ) )}
            </div>
        </div>
    );
}

Use the Item like that:

const Item = ( props ) => {
    const { mpd, onClick } = props;
    const handleClick = () => onClick( mpd.p, mpd.f );

    return (
         <dl onClick={handleClick}>
            <dt>{mpd.p} - {mpd.f}</dt> <dd>{mpd.e} <em>{mpd.c}</em></dd>
         </dl>
    );
}

In this way, you are not using an arrow function in your onClick handler, so this function will not be recreated in every render since we use the reference.

devserkan
  • 16,870
  • 4
  • 31
  • 47
  • Hmm... This actually seems to be slowing things down a little bit for me. Maybe my problem is that I need to pass arguments for each item to the click function, so it has to look like this: `onClick={() => this.clickFunction(mpd.foo, mpd.bar)}`. So I still need to use an arrow function in my onClick handler. (because `onClick={this.clickFunction(mpd.foo, mpd.bar)}` would get executed on render. :-( Any ideas? – Adam D Sep 19 '18 at 10:51
  • How do you decide that this way slows down the things? Don't misunderstand me, I'm just asking to understand the problem. You mean writing so much code by saying slowing? If you want, without a separate component you can use the arrow function as you described here. But, in this way, that function will be recreated in every render. I don't know how much does this affect performance. Some people say much, some people say little. – devserkan Sep 19 '18 at 15:43
  • No, I meant by looking and script and render times in the performance tabs of the chrome developer tools. But then today I tried refactoring a little more and your suggested way now runs a hair faster than my previous setup. (When I first tried it seemed your way was 20-30% slower but I could have been doing something else wrong.) Thank you for your help. I think your way is better for code cleanliness and extensiblity, even if it is only a hair faster. – Adam D Sep 19 '18 at 19:06
  • You are welcome. Yes, separating components is a suggested way in React world. Even in my example `render` method is a little bit crowded :) `.map` can go to in a separate method. Glad that you solved it. – devserkan Sep 19 '18 at 20:19