Emit warning on non-literal content inside of Trans #881
Conversation
|
@nicegamer7 It looks like a breaking change. The last test looks like it was intended to do what it does. Can you please develop a little more why the current code is broken? |
|
@karellm Certainly, my appologies for not including this in the PR initially. After taking a look at this a bit further, it seems I was incorrect. We can't actually extract expressions correctly, unless they're literal values. I'm going to change this to emit a warning in this case. Here's the code sandbox where I determined this: https://codesandbox.io/s/long-water-dggns9?file=%2Fsrc%2FApp.tsx See the debug lines in the console (or the following screenshots) for what the correct extracted value is (the keys that i18next is looking for in the resources). 
So as seen above, neither key is extracted as the expression text, but rather the value. Since we can't extract the value in this tool unless it's a literal, it makes sense to emit a warning. |
|
I'm ok with adding the warning but don't you think that leaving the non-litteral function in the default value provides more context that making it look like it actually worked at extracting it? It is still a breaking change and I want to make sure that we are making the right decision here and not start a ping-pong between both solutions. |
|
Okay, I think that makes sense. I revert most of this and just leave the warning. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #881 +/- ##
==========================================
+ Coverage 93.94% 93.95% +0.01%
==========================================
Files 11 11
Lines 1832 1836 +4
==========================================
+ Hits 1721 1725 +4
Misses 111 111
☔ View full report in Codecov by Sentry. |
nicegamer7
changed the title
|
@karellm Just wanted to bump this; take your time if you're busy, no rush. |
test/lexers/jsx-lexer.test.js
Outdated
| @@ -233,10 +233,10 @@ describe('JsxLexer', () => { | |||
| it('erases tags from content', (done) => { | |||
| const Lexer = new JsxLexer() | |||
| const content = | |||
| '<Trans>a<b test={"</b>"}>c<c>z</c></b>{d}<br stuff={y}/></Trans>' | |||
There was a problem hiding this comment.
Why did you modify theses two tests? Leaving {d} in there is more informative don't you think?
There was a problem hiding this comment.
Hmm, I suppose so. Would you like me to assert that a warning is emitted in these tests as well? Or should I just undo the changes?
There was a problem hiding this comment.
I usually prefer tests that check for a single thing as they are easier to maintain but I don't have a strong opinion either way.
There was a problem hiding this comment.
Okay, I tend to agree. I'll leave it out in that case.
|
Thanks, I just published |
|
Thanks karellm. |
Why am I submitting this PR
We noticed in our project that JSX expressions were being extracted incorrectly. We noticed this by setting
debug: truewhen initializing i18next, then seeing that expressions were extracted as empty non-self-closing tags.Does it fix an existing ticket?
No.
Checklist
yarn test(see details here)