feat(trans): allow interpolation in tOptions if provided #1204

tigerabrodi · 2020-11-30T19:10:53Z

Checklist

only relevant code is changed (make a diff before you submit the PR)
run tests npm run test
tests are included
documentation is changed or added

coveralls · 2020-11-30T19:12:25Z

Coverage remained the same at 95.692% when pulling d8043dc on tigerabrodi:allow-trans-tOptions-interpolation into 0d0f4d8 on i18next:master.

tigerabrodi · 2020-11-30T19:15:09Z

@jamuhl As written on Twitter this morning, I would work on this after work 😃, I am really excited 🥳 This would be my very first OSS contribution 🎉

tigerabrodi · 2020-11-30T19:42:46Z

@jamuhl I have no clue why it is messing with my PR here https://codeclimate.com/github/i18next/react-i18next/pull/1204 🤔 , if anyone else has any clues, would love some assistance 🙌

tigerabrodi · 2020-11-30T19:47:15Z

Do I need to update the documentation in https://github.com/i18next/react-i18next-gitbook, if so, what exactly should I update related to tOptions in https://react.i18next.com/latest/trans-component#trans-props?

src/Trans.js

jamuhl · 2020-12-01T06:59:17Z

src/Trans.js

@@ -298,6 +298,7 @@ export function Trans({
    ...interpolationOverride,
    defaultValue,
    ns: namespaces,
+    ...tOptions?.interpolation,


I think it would be better to change line 293 to add the passed in tOptions.interpolation like:

const interpolationOverride = values ? tOptions?.interpolation : { interpolation: { ...tOptions?.interpolation, prefix: '#$?', suffix: '?$#' } };

Thanks, @jamuhl and @adrai, will make sure to do this change and also add a test, where tOptions.interpolation is passed to Trans 🙌.

Currently working, will do it after work, many thanks again, it means the world to me, my very first OSS contribution 😃 🥰.

…interpolation

tigerabrodi · 2020-12-01T16:32:42Z

@jamuhl @adrai I did the changes as requested 💯, one issue though, it still seems not to be escaping HTML tags properly. Here is a screenshot when using console.log(wrapper.debug()):

I wonder if there is a way to console.log and see the value of tOptions passed into the Trans component 🤔?

Also what do you think of the changes so far? What should I do next?

Many thanks 🙏 💕

adrai · 2020-12-01T17:36:04Z

@tigerabrodi maybe you missed my first comment about optional chaining (?.): #1204 (comment)

regarding the test: I suspect react is already escaping your '<no>Hello' and not the interpolation values of i18next...

tigerabrodi · 2020-12-01T17:41:12Z

@adrai I actually implemented the change @jamuhl suggested #1204 (comment), should I try again, but without optional chaining?

What would the next steps be, since this seems not to be working?
Is there a way to force interpolation to be prioritized over React's escaping HTML tags?

tigerabrodi · 2020-12-01T17:49:50Z

@adrai I pushed a new commit without Optional chaining 👏

adrai · 2020-12-01T18:12:55Z

removal of optional chaining is ok, thx

but the unescape issue is a react problem 🤷‍♂️

I'm not a react expert, maybe someone knows more?

JacobMGEvans · 2020-12-01T18:24:00Z

It could possibly be a test environment issue.
Possibilities:

JSDOM isn't handling it correctly
- Could try updating Jest other related packages
Whatever the test utility library being used isn't rendering it properly
- Could try updating the utility and its configs (i.e. Enzyme, adapters, etc...)
- Could try adding another testing utility library (React Testing Library)
Misconfigured or missing configs for React in Jest
- This would involve some fine combing

@kentcdodds Any other thoughts on this? #1204 (comment)

kentcdodds

I'd suggest switching to React Testing Library eventually, but maybe this is the issue?

kentcdodds · 2020-12-01T18:32:17Z

test/trans.render.spec.js

+  const stringWithHTMLTags = '<no>Hello';
+  const TestElement = () => (
+    <Trans tOptions={{ interpolation: { escapeValue: true } }}>
+      <div> {stringWithHTMLTags} </div>


Suggested change

<div> {stringWithHTMLTags} </div>

<div>{stringWithHTMLTags}</div>

Maybe the extra spaces are the problem?

Oh, I didn't even notice the spaces! 😅

I would try this first, thanks both of you 💕

adrai · 2020-12-01T18:35:42Z

Whatever the test utility library being used isn't rendering it properly
I'd suggest switching to React Testing Library eventually, but maybe this is the issue?

probably it's an enzyme issue...

@tigerabrodi if it works for you in react, I think if this can't be fixed with enzyme, we can skip the test or if you like you can try to switch to a better react test library?

tigerabrodi · 2020-12-01T18:45:26Z

Whatever the test utility library being used isn't rendering it properly
I'd suggest switching to React Testing Library eventually, but maybe this is the issue?

probably it's an enzyme issue...

@tigerabrodi if it works for you in react, I think if this can't be fixed with enzyme, we can skip the test or if you like you can try to switch to a better react test library?

@adrai You mean for that particular test use React Testing Library? Or convert the whole project to use React Testing Library, which I would actually be down to 😃 ?

My suggestion: Try out what @kentcdodds mentioned, if it still does not work, skip the test and merge the changes, and I would with joy and passion convert this project to use React Testing Library instead of Enzyme, which could take some time 👍.

Side question: @adrai I was wondering if this project plans on migrating fully to TypeScript? If so, I'd also be down to help in every way I can 🎉. We use it at work by the way, and you and @jamuhl are just really awesome, being able to contribute here just means the world to me 😃 💕.

adrai · 2020-12-01T18:50:15Z

@tigerabrodiI would say, if you really like to change all tests to "React Testing Library" try it 😉

about TypeScript, idk, maybe not, need to wait for @jamuhl answer tomorrow morning.

tigerabrodi · 2020-12-01T19:01:58Z

@tigerabrodiI would say, if you really like to change all tests to "React Testing Library" try it

about TypeScript, idk, maybe not, need to wait for @jamuhl answer tomorrow morning.

@adrai Is it okay to merge the changes I made in Trans component first and make a new PR with the conversion over to React Testing Library?

I would make a fixup commit and squash the commits, so we only have the changes in Trans component and nothing else 👍.

In a separate PR, I would start the conversion to React Testing Library 🎉. I plan on doing it with @JacobMGEvans, we really look forward to it 💕 🤩

PS. What @kentcdodds pointed out was not the issue 👍.

marcosvega91 · 2020-12-01T19:08:49Z

Good job @tigerabrodi :)

tigerabrodi · 2020-12-01T19:24:48Z

@adrai I pushed the last commit 🙌

Do I still need to change anything or?

I am excited about @jamuhl 's feedback.

Once this gets merged, huge celebration haha 🍾, funny though, my sister has birthday on Friday, what a coincidence, two celebrations in a row 😂 😃.

In another PR, I would start the conversion to React Testing Library 🎉 💕

adrai · 2020-12-01T19:47:07Z

lgtm

jamuhl · 2020-12-02T08:02:23Z

Looks awesome 👍... a great first contribution...keep it up...we're all happy to get help.

Regarding the encoding. This comes from react which does a good job of guarding people against accidentally pass unencoded user input into the page (XSS attacks):

So it comes to two options a dev has to insert translations into a page:

a) using dangerouslysetinnerhtml https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
b) using the Trans component (https://react.i18next.com/latest/trans-component) with it's <Trans><div><no />hello</div></Trans> to string "<0><0 />hello</0>" conversion and interpolation of react children's or named replacement like in <Trans defaults="<no/> hello" components={{ no: <no /> }} />

A little hard to simply describe how the Trans component works / can be used -> which might be a cause of some pain: i18next/react-i18next-gitbook#15

So while this PR fixes an issue - I guess, it will not solve the initial problem you liked to solve. You might open an issue with the content you like to get translated and we might help out with some options on how to use the Trans component for it.

Regarding the switch to React Testing Library that would be the best Xmas present contribution to us, I could think of. If you like to do so - go for it.

Switching completely to typescript is something we considered - @adrai started work on a successor for the current i18next implementation - first in typescript but on the way, we decided to no go further with typescript as it felt more like a burden than a win (not sure it's a lack of our understanding of typescript or really just the price you pay for types, idk).

jamuhl · 2020-12-02T08:09:30Z

Thank you for your contribution - looking forward to see more from you - well done.

This fix was published in react-i18next@11.7.4

tigerabrodi · 2020-12-02T08:43:22Z

@jamuhl hahaha, wohooo 🍾 🎉, you are awesome, I will never forget this moment nor you 🙌 💕

tigerabrodi · 2020-12-02T08:49:50Z

@jamuhl I think regarding TypeScript, that would not be necessary, but React Testing Library, wohooo 🎉, that'd be really awesome, my friends are also down to help me out @kentcdodds @JacobMGEvans and many more 💕

Thank you again @jamuhl, really, my first step into OSS, I admire it so much, I look up to so many, @kentcdodds himself is one of my greatest inspiration and role model, this really means the world to me, I look forward to my long journey here on github haha 😃 💯 🎉 🍾

tigerabrodi · 2020-12-02T09:27:54Z

@jamuhl Regarding the issue we have at work, I would open an issue for that and try to explain it as thoroughly as possible, thank you again, and thank you @adrai also, you guys have no idea how much this means to me!!! 💕 Regarding converting to React Testing Library, that is the least I can do, someday we will all meet up in a React conf hopefully hahaha. This really means so much to me, more than every single birthday that I have had and will have 😃 🎉

tigerabrodi · 2020-12-02T13:50:29Z

@jamuhl Regarding the conversion over to React Testing Library, friends from @kentcdodds Discord also wanna help out 🥳. I plan on opening the PR on Friday, and from there magic will happen 😃 💯, I look forward to it, coding with friends is always awesome 😄.

Thanks to you and @adrai, my first big step into OSS 💕

feat(trans): allow interpolation in tOptions if provided

8e096c1

JacobMGEvans reviewed Nov 30, 2020

View reviewed changes

src/Trans.js Outdated Show resolved Hide resolved

jamuhl requested a review from adrai December 1, 2020 06:59

adrai requested changes Dec 1, 2020

View reviewed changes

src/Trans.js Outdated Show resolved Hide resolved

src/Trans.js Outdated Show resolved Hide resolved

jamuhl reviewed Dec 1, 2020

View reviewed changes

fixup! add test and change interpolationOverride to include tOptions.…

f845489

…interpolation

kentcdodds reviewed Dec 1, 2020

View reviewed changes

feat(trans): allow tOptions.interpolation if passed to tOptions prop

d8043dc

tigerabrodi force-pushed the allow-trans-tOptions-interpolation branch from 4d59b6a to d8043dc Compare December 1, 2020 19:19

adrai approved these changes Dec 1, 2020

View reviewed changes

jamuhl merged commit a5a0167 into i18next:master Dec 2, 2020

tigerabrodi mentioned this pull request Dec 3, 2020

Error occurs on render when using tOptions.interpolation #1205

Closed

dependabot bot mentioned this pull request Mar 15, 2021

Bump react-i18next from 11.5.0 to 11.8.10 marvelousdev0/antdesign#206

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(trans): allow interpolation in tOptions if provided #1204

feat(trans): allow interpolation in tOptions if provided #1204

tigerabrodi commented Nov 30, 2020 •

edited

Loading

coveralls commented Nov 30, 2020 •

edited

Loading

tigerabrodi commented Nov 30, 2020 •

edited

Loading

tigerabrodi commented Nov 30, 2020

tigerabrodi commented Nov 30, 2020 •

edited

Loading

jamuhl Dec 1, 2020

tigerabrodi Dec 1, 2020

tigerabrodi commented Dec 1, 2020

adrai commented Dec 1, 2020

tigerabrodi commented Dec 1, 2020

tigerabrodi commented Dec 1, 2020

adrai commented Dec 1, 2020

JacobMGEvans commented Dec 1, 2020 •

edited

Loading

kentcdodds left a comment

kentcdodds Dec 1, 2020

JacobMGEvans Dec 1, 2020

tigerabrodi Dec 1, 2020

adrai commented Dec 1, 2020

tigerabrodi commented Dec 1, 2020 •

edited

Loading

adrai commented Dec 1, 2020

tigerabrodi commented Dec 1, 2020 •

edited

Loading

marcosvega91 commented Dec 1, 2020

tigerabrodi commented Dec 1, 2020 •

edited

Loading

adrai commented Dec 1, 2020

jamuhl commented Dec 2, 2020

jamuhl commented Dec 2, 2020

tigerabrodi commented Dec 2, 2020

tigerabrodi commented Dec 2, 2020

tigerabrodi commented Dec 2, 2020

tigerabrodi commented Dec 2, 2020 •

edited

Loading

	<div> {stringWithHTMLTags} </div>
	<div>{stringWithHTMLTags}</div>

feat(trans): allow interpolation in tOptions if provided #1204

feat(trans): allow interpolation in tOptions if provided #1204

Conversation

tigerabrodi commented Nov 30, 2020 • edited Loading

Checklist

coveralls commented Nov 30, 2020 • edited Loading

tigerabrodi commented Nov 30, 2020 • edited Loading

tigerabrodi commented Nov 30, 2020

tigerabrodi commented Nov 30, 2020 • edited Loading

jamuhl Dec 1, 2020

Choose a reason for hiding this comment

tigerabrodi Dec 1, 2020

Choose a reason for hiding this comment

tigerabrodi commented Dec 1, 2020

adrai commented Dec 1, 2020

tigerabrodi commented Dec 1, 2020

tigerabrodi commented Dec 1, 2020

adrai commented Dec 1, 2020

JacobMGEvans commented Dec 1, 2020 • edited Loading

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds Dec 1, 2020

Choose a reason for hiding this comment

JacobMGEvans Dec 1, 2020

Choose a reason for hiding this comment

tigerabrodi Dec 1, 2020

Choose a reason for hiding this comment

adrai commented Dec 1, 2020

tigerabrodi commented Dec 1, 2020 • edited Loading

adrai commented Dec 1, 2020

tigerabrodi commented Dec 1, 2020 • edited Loading

marcosvega91 commented Dec 1, 2020

tigerabrodi commented Dec 1, 2020 • edited Loading

adrai commented Dec 1, 2020

jamuhl commented Dec 2, 2020

jamuhl commented Dec 2, 2020

tigerabrodi commented Dec 2, 2020

tigerabrodi commented Dec 2, 2020

tigerabrodi commented Dec 2, 2020

tigerabrodi commented Dec 2, 2020 • edited Loading

tigerabrodi commented Nov 30, 2020 •

edited

Loading

coveralls commented Nov 30, 2020 •

edited

Loading

tigerabrodi commented Nov 30, 2020 •

edited

Loading

tigerabrodi commented Nov 30, 2020 •

edited

Loading

JacobMGEvans commented Dec 1, 2020 •

edited

Loading

tigerabrodi commented Dec 1, 2020 •

edited

Loading

tigerabrodi commented Dec 1, 2020 •

edited

Loading

tigerabrodi commented Dec 1, 2020 •

edited

Loading

tigerabrodi commented Dec 2, 2020 •

edited

Loading