score:3

Accepted answer

Wow, this was a doozy. The ultimate problem stems from the fact that you're calling the following component:

export default function FormsByAudience(props: FormsByAudienceProps) {
  const [critics, setCritics] = useCritics(props.books);

  const setCritic = (index: number) => (critic: IcriticState) => {
    const c = [...critics];

    c[index] = critic;
    setCritics(c);
  };

  // ...
  // in render method:
    setCritic={setCritic(criticIdx)}

for every single different critic (and reader):

props.audiences.critics.map((c) => (
    <div key={c.id}>
        <div>{c.name}</div>
        {props.shouldRenderCritics &&
            props.criticsChildren &&
            cloneElement(props.criticsChildren, { criticId: c.id })}
    </div>
))}

where props.criticsChildren contains <FormsByAudience audienceId={id} books={books} />.

As a result, inside a single render, there are lots of separate bindings for the critics variable. You don't have a single critics for the whole application, or for a given audience; you have multiple critics arrays, one for each critic. Setting the state for one critic's critics array in FormsByAudience does not result in changes to other critics arrays that the other critics' React elements close over.

To fix it, given that the critics array is being created from props.books, the critics state should be put near the same level where the books are used, and definitely not inside a component in a nested mapper function. Then pass down the state and the state setters to the children.

The exact same thing applies to the readers state.

Here is a minimal live Stack Snippet illustrating this problem:

const people = ['bob', 'joe'];
const Person = ({name, i}) => {
  const [peopleData, setPeopleData] = React.useState(people.map(() => 0));
  console.log(peopleData.join(','));
  const onChange = e => {
    setPeopleData(
      peopleData.map((num, j) => j === i ? e.target.value : num)
    )
  };
  return (
    <div>
      <div>{name}</div>
      <input type="number" value={peopleData[i]} onChange={onChange} />
    </div>
  );
};
const App = () => {
    return people.map(
      (name, i) => <Person key={i} {...{ name, i }} />
    );
};

ReactDOM.render(<App />, document.querySelector('.react'));
<script crossorigin src="https://unpkg.com/react@16/umd/react.development.js"></script>
<script crossorigin src="https://unpkg.com/react-dom@16/umd/react-dom.development.js"></script>
<div class='react'></div>

To make the passing of props down easier to type and read, you could consider consolidating the 4 variables (2 data and 2 setters) into a single object.

export default function App() {
  const [critics, setCritics] = useCritics(books);
  const [readers, setReaders] = useReaders(books);
  const dataAndSetters = { critics, setCritics, readers, setReaders };
  const renderByAudience = (id: number) => {
    return <FormsByAudience audienceId={id} {...{ books, dataAndSetters }} />;
  };

Pass dataAndSetters down until you get to

export default function FormsByAudience(props: FormsByAudienceProps) {
  const { critics, setCritics, readers, setReaders } = props.dataAndSetters;

Live demo:

https://codesandbox.io/s/project-forked-woqhf


If I could offer a few other pieces of advice, to up your code quality significantly:

  • Change Recap so that FormsByAudience is imported and called in Recap instead of in the parent. It's really weird (and hard to easily understand at a glance) for renderByAudience to create a React element that sometimes won't ever get used.

  • Recap has the critics and readers switched around. This is probably unintentional. This:

    <h5>Readers</h5>
      {!isEmpty(props.audiences.critics) &&
        props.audiences.critics.map((c) => (
    

    should probably be

    <h5>Readers</h5>
      {props.audiences.readers.map((c) => (
    

    (there's no need for an isEmpty check before mapping an array)

  • The whole

    const selectedReader =
      readers.find((r) => r.readerId === (props.readerId as number)) || {};
    const readerIdx = readers.indexOf(selectedReader as IreaderState);
    const selectedCritic =
      critics.find((r) => r.criticId === (props.criticId as number)) || {};
    const criticIdx = critics.indexOf(selectedCritic as IcriticState);
    

    is unnecessary. Just pass down the reader/critic index from the parent component instead, and use critics[criticIdx] to get to the critic in question.

  • In hook.ts, this:

    const byAudience = books
      .map((b) => b.audiences)
      .flat()
      .filter((bk) => [1].includes(bk.audienceId));
    

    simplifies to

    const byAudience = books
      .flatMap(b => b.audiences)
      .filter(bk => bk.audienceId === 1);
    

    And this:

    byAudience.forEach((el) => {
      el.readers.map((r) => {
        return readersToSet.push({ feedback: "", rate: "", readerId: r.id });
      });
      return readersToSet;
    });
    

    doesn't make sense because forEach ignores its return value, and .map should only be used when returning a value from the callback to construct a new array. Either use flatMap on byAudience, or push inside a for..of loop.

    const readersToSet = byAudience.flatMap(
      el => el.readers.map(
        r => ({ feedback: "", rate: "", readerId: r.id })
      )
    );
    

Related Query

More Query from same tag