Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trans requires objects as children but this is not valid in React #465

Closed
jake-nz opened this issue Jul 3, 2018 · 11 comments
Closed

Trans requires objects as children but this is not valid in React #465

jake-nz opened this issue Jul 3, 2018 · 11 comments

Comments

@jake-nz
Copy link

jake-nz commented Jul 3, 2018

I've always been uneasy about Trans having objects as children like {{ name }} because it's non-standard. In fact React will normally throw an error rendering them "Objects are not valid as a React child..."

Today this caused me problems when testing with enzyme.

import { shallow } from 'enzyme'
const name = "Mock Thing"
const type = "square"
shallow(<Trans i18nKey="test">
  Sorry, you cannot delete the {{ type }} <b>"{{ name }}"</b>
</Trans>)

This results in an error "TypeError: Cannot read property 'children' of undefined" becasue enzyme sees an object with a key 'type' and expects it to also have children (code)

I'm not sure why enzyme does this and perhaps it's something they should fix, but I'd like to suggest here that Trans adopt a React standard aproach to avoid issues. Perhaps this:

<Trans i18nKey="test" type={type} name={{ name , format }}>
  Sorry, you cannot delete the type <b>"name"</b>
</Trans>
@jamuhl
Copy link
Member

jamuhl commented Jul 3, 2018

That way we can't detect were to interpolate (we could do a string.replace - but that not what i18next does).

Still you can:

<Trans
  defaults="hello <0>{{what}}</0>"
  values={{ what: 'world'}}
  components={[<strong>dummyChildren</strong>]}
/>

@jamuhl
Copy link
Member

jamuhl commented Jul 3, 2018

Personally i would prefer having a babel macro converting

<Trans i18nKey="test">
  Sorry, you cannot delete the {type} <b>"{name}"</b>
</Trans>

to

<Trans
  defaults="Sorry, you cannot delete the {{type}} <0>{{name, format}}</0>"
  values={{ type, name }}
  components={[<b>dummyChildren</b>]}
/>

@jamuhl
Copy link
Member

jamuhl commented Jul 3, 2018

Why not doing a string replace in Sorry, you cannot delete the type <b>"name"</b>?

Translators would translate that eg to: Entschuldigen Sie, Sie können den Typ mit <b>"Namen"</b> nicht löschen -> no replacement possible

@jamuhl
Copy link
Member

jamuhl commented Jul 3, 2018

But any suggestion that would would improve this would help.

@BenjaminVanRyseghem
Copy link

BenjaminVanRyseghem commented Aug 22, 2018

To avoid this issue, I ended up modifying Trans so that I could use it like

<Trans i18nKey="userMessagesUnread" count={3} name="Corentin">
    Hello <strong>%name%</strong>, you have %count% unread
    messages. <Link to="/msgs">Go to messages</Link>.
</Trans>
{
  "translation": {
    "userMessagesUnread": "Hello <1>%name%</1>, you have %count% unread message. <3>Go to message</3>.",
}

If that sounds "not too" crazy, I can PR it 😄

edit:

The % is because of

interpolation: {
    prefix: "%",
    suffix: "%"
}

in i18n.init

@jamuhl
Copy link
Member

jamuhl commented Aug 22, 2018

overall why enzyme does not work? it does here: https://github.com/i18next/react-i18next/blob/master/test/trans.render.spec.js#L131

did you mock the Trans component?

Beside...no i guess changing the interpolation prefix/suffix is not the best option. In the end JSX is no valid javascript anyway -> just gets transpiled to it ;)

If needed i would prefer an approach doing the same as we do for the ICU format usecase - ICU is always invalid as a node child -> https://react.i18next.com/misc/using-with-icu-format#using-babel-macros-trans-plural-select

@ioliver
Copy link

ioliver commented Aug 25, 2018

To avoid this issue, I ended up modifying Trans component mock function:

const React = require('react');
const reactI18next = require('react-i18next');

const hasChildren = node =>
  node && (node.children || (node.props && node.props.children));

const getChildren = node =>
  (node && node.children ? node.children : node.props && node.props.children);

const renderNodes = (reactNodes) => {
  if (typeof reactNodes === 'string') {
    return reactNodes;
  }

  return Object.keys(reactNodes).map((key, i) => {
    const child = reactNodes[key];
    const isElement = React.isValidElement(child);

    if (typeof child === 'string') {
      return child;
    } else if (hasChildren(child)) {
      const inner = renderNodes(getChildren(child));
      return React.cloneElement(
        child,
        { ...child.props, key: i },
        inner,
      );
    } else if (typeof child === 'object' && !isElement) {
      return Object.keys(child).reduce((str, childKey) => `${str}${child[childKey]}`, '');
    }

    return child;
  });
};

module.exports = {
  // this mock makes sure any components using the translate HoC receive the t function as a prop
  translate: () => Component => props => <Component t={k => k} {...props} />,
  Trans: ({ children }) => renderNodes(children),
  I18n: ({ children }) => children(k => k, { i18n: {} }),

  // mock if needed
  Interpolate: reactI18next.Interpolate,
  I18nextProvider: reactI18next.I18nextProvider,
  loadNamespaces: reactI18next.loadNamespaces,
  reactI18nextModule: reactI18next.reactI18nextModule,
  setDefaults: reactI18next.setDefaults,
  getDefaults: reactI18next.getDefaults,
  setI18n: reactI18next.setI18n,
  getI18n: reactI18next.getI18n,
};

@jamuhl
Copy link
Member

jamuhl commented Aug 25, 2018

@ioliver cool...makes sense...i updated the sample using your code: https://github.com/i18next/react-i18next/blob/master/example/test-jest/__mocks__/react-i18next.js

@jake-nz
Copy link
Author

jake-nz commented Aug 28, 2018

After updating my react-i18next I'm using the following to do exactly what I want:

<Trans i18nKey="test" values={{type,  name }}>
  Can't delete type <b>"name"</b>
</Trans>

and "test": "Sorry, you cannot delete the {{type}} <b>\"{{name}}\"</b>"

@jamuhl
Copy link
Member

jamuhl commented Aug 28, 2018

@jake-nz ok that works too ;) -> just would not have created the right string for translations at start using the save missing feature. If it works for you - that's great.

@jamuhl
Copy link
Member

jamuhl commented Sep 13, 2018

closing this....if still an issue - feel free to reopen this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants