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

Fix tag converting behavior to match with react-i18next #665

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

yoo2001818
Copy link

Why am I submitting this PR

This PR is continuation of #663. (You can disregard #663 if this PR gets merged first)

I've checked the difference between react-i18next and i18next-parser, and mainly there were 3 problems.

This resolves 1st and 3rd point to match with react-i18next's behavior.

I think #264 can be resolved if this PR gets merged, and transSupportBasicHtmlNodes becomes true by default. However, since that would be a breaking change, a proper documentation about transSupportBasicHtmlNodes could be enough to resolve it. I'll try to update the documentation if you want to.

Does it fix an existing ticket?

No

Checklist

  • only relevant code is changed (make a diff before you submit the PR)
  • tests are included and pass: yarn test (see details here)
  • documentation is changed or added

@mskrzypek
Copy link

mskrzypek commented Dec 29, 2022

I think #264 can be resolved if this PR gets merged, and transSupportBasicHtmlNodes becomes true by default.

@yoo2001818 transSupportBasicHtmlNodes is true by default, isn't it? https://react.i18next.com/latest/i18next-instance

@yoo2001818
Copy link
Author

yoo2001818 commented Dec 29, 2022

I think #264 can be resolved if this PR gets merged, and transSupportBasicHtmlNodes becomes true by default.

@yoo2001818 transSupportBasicHtmlNodes is true by default, isn't it? https://react.i18next.com/latest/i18next-instance

No, it is true by default in i18next itself, but i18next-parser defaults to false (https://github.com/i18next/i18next-parser/blob/master/src/lexers/jsx-lexer.js#L10).

You have to match them, either by setting i18next's value to false (which will work perfectly for now), or setting i18next-parser's value to true. (which this PR tries to resolve discrepancies)

@mskrzypek
Copy link

You have to match them, either by setting i18next's value to false (which will work perfectly for now), or setting i18next-parser's value to true. (which this PR tries to resolve discrepancies)

As far as i can see, i18next-parser does not have transSupportBasicHtmlNodes setting itself. Correct me if I'm wrong, but It is derived from i18next settings, with default (fallback) to false indeed.

@yoo2001818
Copy link
Author

You have to match them, either by setting i18next's value to false (which will work perfectly for now), or setting i18next-parser's value to true. (which this PR tries to resolve discrepancies)

As far as i can see, i18next-parser does not have transSupportBasicHtmlNodes setting itself. Correct me if I'm wrong, but It is derived from i18next settings, with default (fallback) to false indeed.

i18next-parser does have transSupportBasicHtmlNodes setting, however it is undocumented for now.

You may use it like this:

// i18next-parser.config.js
export default {
  lexers: {
    jsx: [{
      lexer: 'JsxLexer',
      transSupportBasicHtmlNodes: true,
    }],
  },
};

Furthermore, i18next-parser cannot read i18next's config, as i18next-parser statically analyzes the code without executing it. It doesn't actually care about i18next's configuration or its presence - it works by just reading t function call (or <Trans>) in the code. So it's the developer's job to match these config.

However, i18next itself, does set transSupportBasicHtmlNodes to true by default. What I've linked was for i18next-parser, which sets the config to false by default.

@karellm karellm force-pushed the master branch 2 times, most recently from d72e758 to 10f3d69 Compare May 8, 2023 03:17
@jaycle
Copy link

jaycle commented May 30, 2023

Bumped into this today and was so grateful to find not only is it reported, but there's a PR already. Could this be looked at by a maintainer?

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

Successfully merging this pull request may close these issues.

3 participants