DEV Community

Discussion on: My first React 'aha' moment. Is this an antipattern?

Collapse
 
speculative profile image
Jeffrey Tao

For the general pattern, you've got it right. HTML is a render-time concern, and your state should be more concerned with pure data. One of the key benefits of React is that you no longer need to manually inspect and manipulate HTML.

One detail:

    {this.state.output.map(
        (obj, index) =>
          obj.type === 'stdout' ? (
            <Stdout key={index}>{obj.data}</Stdout>
          ) : (
            <Stderr key={index}>{obj.data}</Stderr>
          )
    )}

You want to be careful about how you specify keys when returning an array of components. From the React Docs:

When you don’t have stable IDs for rendered items, you may use the item index as a key as a last resort ... We don’t recommend using indexes for keys if the order of items may change. This can negatively impact performance and may cause issues with component state.

They key needs to uniquely identify a component among its siblings. In this case, as long as your output scrollback grows indefinitely, keys do uniquely identify elements (since they basically correspond to a unique counter of log lines), but if you were to do something like limit scrollback to the last N lines, what would happen to the element at index 0? As soon as you add an N+1th line, the element at index 1 is now index 0, and we've violated the unique key rule because a component has changed keys.

That's a comparatively small nitpick, though. The before/after you've written in the data-centric refactor is definitely a large step in the right direction.

Collapse
 
theundefined profile image
Joe

I did read about that in the docs and dismissed it as something I didn't need to worry about for this scenario, but as you've pointed out, I could end up running into this issue if I implemented a max size on the output. So thanks for highlighting this to me, nitpicking is helpful :)

Hmmm, so I guess to get a unique key in this scenario I could increment an id counter when appending objects to the output collection, like a primary key. The db analogy seems to help me figure this stuff out lol.

On the note of nitpicking: I turned on some linters yesterday and got a bunch of warnings, so I've found a few ways to improve the code in the example. I think the main one is not accessing state when trying to update it, so I've moved concatenating the output collection to a callback:

moku.stdout.on('data', data => {
  if (data.length > 0) {
    this.setState(prevState => ({
      isRunning: true,
      output: prevState.output.concat({
        type: 'stdout',
        data: data.toString(),
      }),
    }));
  }
});

I've also learnt about destructing arguments. These linters are very educational :p