Skip to content

Bug(messageformat): Plural translation starting and ending with a variables fails #621

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

Open
1 task done
julpellat opened this issue Jan 27, 2023 · 7 comments
Open
1 task done

Comments

@julpellat
Copy link

julpellat commented Jan 27, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Which Transloco package(s) are the source of the bug?

@ngneat/transloco-messageformat

Is this a regression?

No

Current behavior

I have the following translation using plural in my en.json file :

"MONITORING_CALLOUT": "{count, plural, \none {{count} manager n’a pas réalisé sa déclaration de variables à la date d'échéance pour la période de {period}} \nother {{count} n'ont pas réalisé leur déclaration de variables à la date d'échéance pour la période de {period}}\n}"

The page load but nothing show on the screen. Nor the value, nor the translation key. And I have the following error in the console

ERROR Error: invalid syntax at line 2 col 1:

  one  
  ^
    _token messageformat.js:3477
    next messageformat.js:3419
    next messageformat.js:3490
    n messageformat.js:279
    parseSelect messageformat.js:3860
    parseArgToken messageformat.js:3960
    parseBody messageformat.js:3983
    parse messageformat.js:3828
    parse messageformat.js:4044
    compile messageformat.js:4510
    compile messageformat.js:7216
    compile ngneat-transloco-messageformat.mjs:22
    transpile ngneat-transloco-messageformat.mjs:52
    translate ngneat-transloco.mjs:597
    updateValue ngneat-transloco.mjs:1338
    subscription ngneat-transloco.mjs:1325
    RxJS 43
    transform ngneat-transloco.mjs:1325
    pipeBind2 Angular
    MonitoringPage_ng_container_9_div_1_Template monitoring.page.html:21
    Angular 24
    RxJS 6
    Angular 20
    RxJS 16

After some testing my conclusion are :

  • when there is a variable at the start and at the end the parsing throws an error
    Example : "{count, plural, \none {bla bla} \nother {{var1} bla bla {var2}}"
  • If I had a spaces it works fine
    Like this "{count, plural, \none {bla bla} \nother {{var1} bla bla {var2} }" or at the start "count, plural, \none {bla bla} \nother { {var1} bla bla {var2}}"
  • If I use the syntax with a # it also works fine
    "{count, plural, \none {bla bla} \nother {# bla bla {var2}}"

Expected behavior

The syntax "KEY": "count, plural, \none {bla bla} \nother {{var1} bla bla {var2}}" should not throws any error.

Please provide a link to a minimal reproduction of the bug

https://stackblitz.com/edit/transloco-messageformat-giflos?file=src%2Fassets%2Fi18n%2Fen-US.json,src%2Fapp%2Fapp.component.html,src%2Fapp%2Fapp.module.ts,src%2Findex.html

Transloco Config

No response

Please provide the environment you discovered this bug in

"@ngneat/transloco": "^4.2.1"
"@ngneat/transloco-messageformat": "^4.1.0"
Angular: 15.0.4
Node: 18.12.1
Package Manager: npm 8.19.2
OS: Windows 10

Browser

Firefox: version 109.0 (64 bits)

Additional context

No response

I would like to make a pull request for this bug

No

@bolt-new-by-stackblitz
Copy link

Fix this issue in StackBlitz Codeflow Start a new pull request in StackBlitz Codeflow.

@ben12
Copy link
Contributor

ben12 commented Mar 5, 2023

There is a conflict between transloco and messageformat patterns.

For {count, plural, one {bla bla} other {{var1} bla bla {var2}}}, transloco match {{var1} bla bla {var2}} and search a value for var1} bla bla {var2 which is not found, so messageformat receive {count, plural, one {bla bla} other var1} bla bla {var2} and returns a syntax error.

A workaround is to use another pattern for transloco by replacing the default configuration of interpolation.

The final solution may be to add forbidden characters for parameters name:

export interface TranslocoConfig {

[...]

  interpolation?: [start: string, end: string, forbiddenChars?: string];
}

By default it may be interpolation: ['{{', '}}', "{}"]. So interpolation regex will become {{([^{}]*?)}} instead of {{(.*?)}}. It is a breaking change because '{' and '}' cannot be used anymore for parameters name, but i think that is an error to use them.

It can also be nice to escape regex reserved characters:

 function resolveMatcher(userConfig?: TranslocoConfig): RegExp {
-  const [start, end] =
+  const [start, end, forbiddenChars] =
     userConfig && userConfig.interpolation
       ? userConfig.interpolation
       : defaultConfig.interpolation!;
+  const matchingParamName = forbiddenChars !== undefined ? `[^${escapeForRegExp(forbiddenChars)}]` : '.';
+  return new RegExp(`${escapeForRegExp(start)}(${matchingParamName}*?)${escapeForRegExp(end)}`, 'g');
-  return new RegExp(`${start}(.*?)${end}`, 'g');
}

+function escapeForRegExp(text: string) {
+  return text.replace(/[-[\]{}()*+?.,\\^$|#\s]/g, "\\$&");
+}

What do you think about this ?

@shaharkazaz
Copy link
Collaborator

@julpellat What's wrong with the workarounds you provided?
You can try @ben12's suggestion and pass a custom interpolation value.

@julpellat
Copy link
Author

Hi, sorry for the late response.
I’ve tested your solution and it work like a charm. This is exactly what I need.

To answer your question @shaharkazaz I can’t use the workarounds I provided because we use lokalize and lokalize generate the json file used by transloco.
Lokalize generate plural under this syntax "{count, plural, \none {bla bla} \nother {{var1} bla bla {var2}}"
It will be way easier for us to fix messageformat instead of changing the lokalize config and refactor all the plural syntax in all of our software (since lokalize handle the translation for all of our software)

ben12 added a commit to ben12/transloco that referenced this issue Mar 25, 2023
ben12 added a commit to ben12/transloco that referenced this issue Mar 25, 2023
ben12 added a commit to ben12/transloco that referenced this issue Mar 25, 2023
ben12 added a commit to ben12/transloco that referenced this issue Mar 25, 2023
ben12 added a commit to ben12/transloco that referenced this issue Apr 1, 2023
@julpellat
Copy link
Author

julpellat commented May 16, 2023

Hi, the PR seems to be ready since late march. Is there still some work needed to merge ? Can I help in any way ?

@JMCelesti
Copy link

JMCelesti commented Aug 2, 2023

@julpellat I was looking since yesterday all day long and I found the solution
not just for you but for the other looking for the solution
the none case: =0 is mandatory

I have on my file =0 {} one{just one} other{more than one}

@shaharkazaz
Copy link
Collaborator

@julpellat I need to fully understand the use case and make sure this isn't adding a bunch of code to a very specific use case. (seeing @JMCelesti comment)
I didn't have time to dive into it, I'll try to take a look when I get the chance.

ben12 added a commit to ben12/transloco that referenced this issue Aug 6, 2023
ben12 added a commit to ben12/transloco that referenced this issue Aug 6, 2023
ben12 added a commit to ben12/transloco that referenced this issue Aug 6, 2023
ben12 added a commit to ben12/transloco that referenced this issue Aug 6, 2023
ben12 added a commit to ben12/transloco that referenced this issue Aug 6, 2023
ben12 added a commit to ben12/transloco that referenced this issue Aug 6, 2023
ben12 added a commit to ben12/transloco that referenced this issue Aug 6, 2023
ben12 added a commit to ben12/transloco that referenced this issue Oct 26, 2023
ben12 added a commit to ben12/transloco that referenced this issue Oct 26, 2023
ben12 added a commit to ben12/transloco that referenced this issue Oct 26, 2023
ben12 added a commit to ben12/transloco that referenced this issue Oct 26, 2023
ben12 added a commit to ben12/transloco that referenced this issue Oct 26, 2023
ben12 added a commit to ben12/transloco that referenced this issue Oct 26, 2023
ben12 added a commit to ben12/transloco that referenced this issue Oct 26, 2023
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