-
Notifications
You must be signed in to change notification settings - Fork 5
Remove invalid react
property i18n config
#54
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me esp since the prior anchor in the docs is outdated, however might be worth @rocketnova taking a look in case they have context from why we initially added this that we may be missing!
Good call, will add Rocket as a reviewer! @rocketnova no urgency on this at all, just let us know whether or not this looks okay whenever you get a minute. Thanks! |
@@ -2,10 +2,5 @@ module.exports = { | |||
i18n: { | |||
defaultLocale: "en", | |||
locales: ["en", "es"], | |||
react: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's poorly documented but it looks like the fix, while preserving the safelisting of em
, is to unnest this react
object outside of the i18n
object:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you've removed your blocking review, just for the record, I'm fine with this approach too; it's one of those cases where I don't quite understand why em
isn't included by default, so it's just a question of whether to go with the default because it's the default, or with something else that seems to make more sense. Since it's not likely to matter either way, I'm still thinking we stick with the default in the template. I'll leave this open for another day or so in case Rocket wants to chime in, and then I'll close this out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There may be reason for an individual application to use that setting, but I don't think the template needs to have it by default, and either way, the option was being set improperly anyway, so I opted to remove the react property altogether.
Sorry, I should have read your PR description before reviewing! I'm fine with this, so removing my blocking review.
Ticket
resolves #51
Changes
Removed the
react
property from thei18n
property innext-i18next.config.json
.Context for reviewers
When running the build, prior to this change, the output would show:
The problematic config looked like this:
I'm not aware of a good reason for that setting to be set generally in the template; the link in the comment was outdated, so the anchor it tried to connect to no longer exists, but at the top of that page, it says "the truth is: in most cases you don't even need it" in reference to the
Trans
component thetransKeepBasicHtmlNodesFor
setting connects to. There may be reason for an individual application to use that setting, but I don't think the template needs to have it by default, and either way, the option was being set improperly anyway, so I opted to remove thereact
property altogether.Testing
Ran
npm run test
andnpm run dev
and everything seems to work as normal. Runningnpm run build
no longer shows the error message referenced in the issue.