i18next / react-i18next

Internationalization for react done right. Using the i18next i18n ecosystem.
https://react.i18next.com
MIT License
9.21k stars 1.02k forks source link

`Trans` component should NOT modify the input props value. #1733

Open Jayatubi opened 6 months ago

Jayatubi commented 6 months ago

I've got an issue around this line in TransWithoutContext.js:

https://github.com/i18next/react-i18next/blob/b274be772bde64377ce1d75f28c7cd3f46735851/src/TransWithoutContext.js#L380

  if (components) {
    Object.keys(components).forEach((c) => {
      const comp = components[c];
      if (
        typeof comp.type === 'function' ||
        !comp.props ||
        !comp.props.children ||
        (translation.indexOf(`${c}/>`) < 0 && translation.indexOf(`${c} />`) < 0)
      )
        return;

      // eslint-disable-next-line react/no-unstable-nested-components, no-inner-declarations
      function Componentized() {
        // <>{comp}</>
        return createElement(Fragment, null, comp);
      }
      // <Componentized />
      components[c] = createElement(Componentized);
      // ^ `components` may be frozen and then crash happens
    });
  }

Looks like Trans was going to modify the input prop components inplace. However, this may lead a crash while the components is readonly, aka frozen.

I encounter this issue by accidentally storing a React component with Trans by using useImmer hook. The useImmer hook will freeze all the states in production environment by default.

For exmaple:

const MyComponent = () => {
    // The components props will be frozen in production environment
    const [Inner, setInner] = useImmer(<>
        <Trans components={components}></Trans>
    </>)

    return <Inner/>;
}

So instead of using forEach to convert the input components may be it would be better to use map to create a new variable for later use.

adrai commented 6 months ago

Feel free to provide a PR to address this.