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

Using Trans inside a Plural macro unexpectedly replaces "#" found in variables #1826

Closed
kiejo opened this issue Jan 3, 2024 · 5 comments · Fixed by #1850
Closed

Using Trans inside a Plural macro unexpectedly replaces "#" found in variables #1826

kiejo opened this issue Jan 3, 2024 · 5 comments · Fixed by #1850

Comments

@kiejo
Copy link

kiejo commented Jan 3, 2024

Describe the bug
Using Trans with the Plural macro can lead to variables being rendered in a modified way. It should be possible for a variable to contain "#" without it being replaced with the value of the Plural macro.

To Reproduce
Define a component, which uses a nested Trans within the Plural macro:

import {Plural, Trans} from '@lingui/macro'

function Notification({count, documentTitle}) {
  return (
    <p>
      <Plural
        value={count}
        one={<Trans>There is a notification in <span>{documentTitle}</span></Trans>}
        other={<Trans>There are # notifications in <span>{documentTitle}</span></Trans>}
      />
    </p>
  )
}

Expected behavior
Rendering the component using this code:

<Notification
  count={1}
  documentTitle="Title #1"
/>

Should result in

<p>There is a notification in <span>Title #1</span></p>`

but it generates

<p>There is a notification in <span>Title 11</span></p>

As you can see, the "#" found in the documentTitle variable is replaced with the count "1". It renders as Title 11 instead of Title #1.

I think the expected behavior is to leave variables untouched and render them as is. Only hashtags that are directly part of the translation string should be replaced.

Additional context

  • jsLingui version 4.6.0
  • Babel version 7.23.3
  • I'm using Babel with babel-macro-plugin
@jozefmery
Copy link
Contributor

Hi, I investigated this issue and I believe the core of the issue is that the interpolation here runs before the replaceOctothorpe function that uses the String#replace method. By interpolating first, there is no easy way of knowing where the # originated from in the component hierarchy. Furthermore, by using the String#replace method, only the first occurence is replaced as the provided # pattern here is a string. Currently, there is no escape hatch for the replacement of the # symbol that I know of.

To solve this issue I propose the following: Remove the # related behavior entirely

I believe that a better and simpler solution to include the actual number in a plural sentence is to use the well-established templating approach of Lingui (e.g.: There are ${count} apples). replaceOctothorpe additionally formats the number for you (number style by default), but you may do that as you would anywhere else: There are ${i18n.number(count, ...)} apples. This approach covers more (albeit edge-case) use-case as it allows you to include the number any times, and eliminates the issues related to a bespoke templating systems that sort of goes against the philosophy of the library.

I would love to hear your feedback and please let me know if I missed anything.

@thekip
Copy link
Collaborator

thekip commented Feb 13, 2024

Thanks for digging into the issue, the "octothorpe" symbol is part of ICU messages spec, so that's why there are here.

@jozefmery
Copy link
Contributor

Thank you for the rapid response. You are correct I did not consider that, so the replacing of the # is correct, except I believe every instance should be replaced. Furthmore, literal # and any other special symbols are supported by enclosing them in single quotes '#' and a single quote is represented by two consecutive ones ''. It is described here. You can check out this online editor.

thekip added a commit to thekip/js-lingui that referenced this issue Feb 14, 2024
@thekip
Copy link
Collaborator

thekip commented Feb 14, 2024

will be fixed by #1850

@kiejo
Copy link
Author

kiejo commented Feb 16, 2024

Thanks for fixing this! I'm looking forward to the release which will include the fix :)

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

Successfully merging a pull request may close this issue.

4 participants