Prettier can break <Trans/>

[timhwang21]

Hi, I'm reporting an undesirable interaction between Prettier and <Trans/>. The frustrating part is that I think both are behaving as expected, so I am not sure if this is even possible to fix. Would be interested in advice to best avoid this!

<Trans/> uses tagged strings like '<0>first node</0> second node <2>third node</2>' to translate content like <b>bold</b> unbolded <i>italicized</i>.

Prettier formats long blocks of text in HTML by inserting {' '}, which is a fairly standard way of adding spaces between nodes in React. However, this added space is also treated as a node by <Trans/>.

Thus, the following are not equivalent, even though Prettier may silently format the first into the second:

/*
  <0/> is <b>bold</b>
  <1/> is ' unbolded '
  <2/> is <i>italicized</i>
 */
<Trans>
  <span>
    <b>bold</b> unbolded <i>italicized</i>
  </span>
</Trans>

/*
  <0/> is <b>bold</b>
  <1/> is ' '
  <2/> is 'unbolded'
  <3/> is ' '
  <4/> is <i>italicized</i>

  Prettier will insert the spaces if these strings are very long.
 */
<Trans>
  <span>
    <b>bold</b>
    {' '}unbolded{' '}
    <i>italicized</i>
  </span>
</Trans>

The sneaky part of this issue is that you can be testing your change, verify that it works, run your formatter (which silently breaks the translation), and push your change.

[jamuhl]

the only thing i could think of: https://prettier.io/docs/en/ignore.html

not sure how we could monkey patch that on our side - how we decide if that {' '} was added intentional or just out of prettier?!?

[timhwang21]

Thanks for the quick response!

I'm not really sure myself... I am wondering how advisable it might be to just have <Trans/> ignore {' '} nodes.

My understanding is that actually having content in<Trans/> is not needed for the translation to work (e.g. we can do <Trans><b>0</b>1<i>2</i></Trans>), and is only done for helping with filling missing translations?

So this change shouldn't actually change anything? Unless I'm missing something :)

[jamuhl]

the JSX nodes to interpolate need to have the right index the string parts in between are irrelevant (as long the index in children is right)

[timhwang21]

Right -- I was saying because the string parts don't matter, if we detect {' '}hello world{' '} it seems safe to just interpret that as a single node hello world.

I believe this case only causes an issue when the {' '} is next to a plain string -- <b>Hello</b>{' '}<i>World</i> and <b>Hello</b> <i>World</i> are identical as far as i18next is concerned, so that won't be affected.

[jamuhl]

yes ... just {' '} is an additional node so changing index of other needed jsx elements to interpolate. Ignoring those {' '} in our implementation is not as easy as we would need to recalculate indexes.

[timhwang21]

In Trans.js#L18 we have this block that checks the incoming nodes. WDYT about adding some logic like "if current node is the empty string, and EITHER the previous/next nodes are plain strings, ignore the node"?

Test cases:

1. <b>Foo</b>{' '}Bar{' '}<b>Baz</b> -- the spaces are both combined with 'Bar'
2. <b>Foo</b>{' '}<b>Baz</b> -- nothing happens; space remains node 1 and seccond bold remains node 2.
3. Foo{' '}Baz -- the space is combined with either Foo or Bar, it doesn't matter which. As long as 'Foo' and 'Bar' remain distinct nodes.

Can also maybe add a flag ignoreSpaces to ensure backwards compatibility? If this sounds like an acceptable solution (e.g. I am not missing anything that would cause this to heavily break things) I can take a crack at this!

[jamuhl]

I will keep this open here for the case more devs ask for it.

But my personal opinion: "Do not monkey patch a package - just because a code style formatter tool breaks it" <-- Feels absolutely wrong.

[timhwang21]

That's fair! Thanks for the quick responses 🙏

My perspective is that this affects more than just a code formatter, {' '} is a fairly common idiom in React to add whitespace between elements due to how JSX handles the newline char.

[jamuhl]

closing this for now...seems not to attract interest of people...

[nikolay-borzov]

Perhaps we should mention workaround in the documentation.

[amitava82]

I'm sorry but what's the solution/workaround here?

[jamuhl]

You can use Trans like https://react.i18next.com/latest/trans-component#alternative-usage

I got no idea how we should detect changes made by prettier breaking translations

[joyfulelement]

Bumped into the same issue with prettier + react.i18next + @testing-library/react.

Here was another example:

A one liner with <a> tag:

<p><Trans i18nKey={'previewText.learnMore'}>Learn more <a href="/docs/about" target="_blank" rel="noopener" className={styles.previewDocLink}>here</a>.</Trans></p>

was turned into the following by prettier:

<p>
  <Trans i18nKey={'previewText.learnMore'}>
    Learn more{' '}
    <a
      href="/docs/about"
      target="_blank"
      rel="noopener"
      className={styles.previewDocLink}
    >
      here
    </a>
    .
  </Trans>
</p>

and would break the unit test, causing the entire anchor <a/> tag being missing:

TestingLibraryElementError: Unable to find an accessible element with the role "link"
    There are no accessible roles. But there might be some inaccessible roles. If you wish to access them, then set the `hidden` option to `true`. Learn more about this here: https://testing-library.com/docs/dom-testing-library/api-queries#byrole
    <div
      class="previewInfoWrapper"
      data-testid="info"
    >
      <div
        class="previewInfoIcon"
        role="img"
      />
      <div
        class="previewText"
      >
        <p>
          Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. 
        </p>
        <p>
          Learn more 
          .
        </p>
      </div>
    </div>
      172 |     const infoContainer = getByTestId('info');
      173 | 
    > 174 |     const docsLink = within(infoContainer).getByRole('link');
          |                                            ^

Perhaps we need to mention such compatibility issue on the documentation to prevent people from being caught from surprise when using <Trans/> with Prettier.
Other ref: prettier/prettier#4223

[whydidoo]

Recently I have written a package for eslint(plugin). It adds a /* prettier-ignore*/ for all components