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

Failing recommended config with valid translations? #7

Open
mrlubos opened this issue Apr 6, 2018 · 11 comments
Open

Failing recommended config with valid translations? #7

mrlubos opened this issue Apr 6, 2018 · 11 comments
Labels
i18n-next Issues involving support for i18n-next

Comments

@mrlubos
Copy link

mrlubos commented Apr 6, 2018

Hello,

I have just found out about this plugin and decided to test it on a working project. For the sake of example, I will simplify my translation files down to the following file locales/translation.json.

{
  "too_long": "Max {{ limit }} characters allowed!"
}

Then in .eslintrc.json I added this line to the extends array.

"extends": [
    "config-1",
    "plugin:i18n-json/recommended"
]

And added this script to package.json.

"scripts": {
    "lint-i18n": "eslint --fix --ext .json --format node_modules/eslint-plugin-i18n-json/formatter.js locales/"
}

Let's execute yarn lint-i18n now...

✖  ERROR  (i18n-json/valid-message-syntax)

  - Expected
  + Received

    Object {
  -   "too_long": "ValidMessage<String>",
  +   "too_long": "String('Max {{ limit }} characters allowed') ===> Expected '0', [1-9] or [^ tnr,.+={}#] but '{' found.",
    }

> ✖ 1 ERROR
> ⚠ 0 WARNINGS

How do I make this test pass? Whenever this translation is used, it gets injected the limit variable. Thanks for any advice!

@mayank23
Copy link
Contributor

mayank23 commented Apr 6, 2018

Hi @lmenus 😄, we support ICU Message Syntax by default. Nice guide on it: https://formatjs.io/guides/message-syntax/. (React-Intl uses ICU Message Syntax)

Which message syntax are you using? looks like i18n-next? If so, this is on the plan to be supported next. (Maybe you could help with a PR? 😉 )

Another way to get this working/test around is by writing a customer message syntax validator as shown in this example: https://github.com/godaddy/eslint-plugin-i18n-json/tree/master/examples/custom-message-syntax

Hope this helps!

@mrlubos
Copy link
Author

mrlubos commented Apr 6, 2018

Hi @mayank23, yes and yes, those are good guesses! I only created this issue because it didn't feel right to use a recommended setting and have to write my own syntax validator! Right now it fails my linter script so I had to disable it. 😬 Will see about the PR, has anyone started working on it already?

@mayank23
Copy link
Contributor

mayank23 commented Apr 6, 2018

Planning on adding support for i18n-next very soon 😄 .

As a temporary measure, you could override the recommended settings to disable the syntax rule or use the built-in non-empty-string validator like so:

The non-empty-string validator checks if the message value is of type String and not empty.

screen shot 2018-04-06 at 12 48 58 pm

Example config with builtin non-empty-string validator:

  // .eslintrc.json
  "extends": [
    "plugin:i18n-json/recommended"
  ],
  "rules": {
    "i18n-json/valid-message-syntax": [2,{
      "syntax": "non-empty-string"
    }]
  }

Example config with the syntax rule disabled .

  // .eslintrc.json
  "extends": [
    "plugin:i18n-json/recommended"
  ],
  "rules": {
    "i18n-json/valid-message-syntax": 0
  }

@mrlubos
Copy link
Author

mrlubos commented Apr 6, 2018

Thank you! Are you going to support objects (including arrays) as keys in the i18next integration?

@mayank23
Copy link
Contributor

mayank23 commented Apr 7, 2018

I have to look more closely at i18n-next, but yes nested objects will be supported definitely.
Could you give an example where you use arrays?

Thanks!

@mrlubos
Copy link
Author

mrlubos commented Apr 7, 2018

Sure. React/JSX syntax supports arrays as arguments and I found it useful to add <br /> tags to text. Instead of doing complicated HTML parsing or using dangerouslySetInnerHTML and potentially exposing the application to XSS attacks, I can define an array in the translation files where every string represents a single line. For example, translation.json.

"translationArray": [
  "line1",
  "line2"
]

Then in React this would get rendered like so.

render() {
  let lines = [];
  translationArray.forEach((line, index) => {
    lines = [
      ...lines,
      line,
      <br key={index} />,
    ];
  });
  return lines;
}

Result

<p>
  line1 <br />
  line2 <br />
</p>

This is used specifically for plain text that just needs to be spread across multiple lines. If we were to add more rich text editing features, this solution wouldn't be very useful.

@mayank23
Copy link
Contributor

mayank23 commented Apr 9, 2018

Awesome, thanks for the example 😉

@mayank23 mayank23 added the i18n-next Issues involving support for i18n-next label Apr 9, 2018
@silentsakky
Copy link

Maybe we can use https://github.com/i18next/i18next-translation-parser to add support for i18next format, let me know if that works I can give it a try

@mayank23
Copy link
Contributor

Hi @silentsakky :) , does that library tell us if the syntax is incorrect? I did a quick scan and couldn't find any error handling code in that library

@ravinggenius
Copy link

@mayank23 I wrote a quick message validator using i18next-translation-parser. For my purposes I can validate a string is formatted correctly, but having the hard-coded check for arrays is a deal-breaker. Also I can't validate null, which I use to indicate messages that don't require translation (the region is locked out of that part of the UI which would use the translation).

@ravinggenius
Copy link

@mayank23 to answer your question though (oops!), I never saw i18next-translation-parser throw an error, so my validation function had to do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n-next Issues involving support for i18n-next
Projects
None yet
Development

No branches or pull requests

4 participants