fix: react-i18next, added better support for namespace overrides, corrected e2e-tests #871
Conversation
|
@beinarovic, @antfu please give this a look |
|
@anilkk could you take a look at this? |
|
any info on the review of this PR? |
Co-authored-by: Michael Overmeyer <michael@overmeyer.io>
|
@movermeyer Would you say that this is a better fix than #873? |
src/frameworks/react-i18next.ts
Outdated
| keypath, | ||
| ] | ||
| } | ||
|
|
||
| rewriteKeys(key: string, source: RewriteKeySource, context: RewriteKeyContext = {}) { | ||
| const dotedKey = key.split(this.namespaceDelimitersRegex).join('.') |
There was a problem hiding this comment.
AFAICT, during i18n-ally.extract-text, keys with either : or / will fail to get here, since they will be caught by this call to keypathValidate.
This isn't necessarily a problem for this PR (it works for the other uses), but something that'll need to be fixed in the future in order to get things working 100% with react-i18n. (Just a note for myself really)
| if (Global.namespaceEnabled) { | ||
| namespace = node?.meta?.namespace || namespace | ||
| if (namespace) | ||
| if (namespace && keypath.startsWith(namespace + delimiter)) | ||
| return keypath.slice(namespace.length + 1) |
There was a problem hiding this comment.
[Not this PR's fault]
Shouldn't this do the slice based on the length of the delimiter (for robustness). Can we assume that a delimiter will always be a single character?
(Just a note for myself really)
Co-authored-by: Michael Overmeyer <michael@overmeyer.io>
While both solve the failing build problem, they otherwise solve different problems. Once the build is working again, we can evaluate the other changes that #873 offers. |
|
@Swiftwork Could you please fix the build and address the code comments? Then this will be merged |
I think getting the tests to pass is just a matter of: But I don't have the permissions to push to your branch |
|
I have updated the snapshot. However, as it, unfortunately, was a while ago since I wrote the code and the PR, I can't remember the relevance of dottedKey, but I believe it was copied from existing code. |
|
@Swiftwork tests are still failing |
|
@kibertoad Correct me if I am wrong, but these test failures look unrelated to my code and seemingly have something to do with Vue. |
|
@movermeyer Do you think test failures should be addressed in a separate PR? |
@kibertoad I agree, these test failures are independent of this PR. Let's merge this and deal with those failures separately. |
|
@Swiftwork thanks a lot for your contribution! |
|
It seems like the plural (_one, _other) still doesn't work 😂 or maybe I've missed some configurations... |
|
Same with plural v4 format with i18next v23.4.4 and react-i18next v13.1.2 |
…rected e2e-tests (lokalise#871) Co-authored-by: Igor Savin <iselwin@gmail.com> Co-authored-by: Michael Overmeyer <michael@overmeyer.io>
This pull request clears up the issues introduced by #735 and improves upon the
react-i18nextframework.fluent-vue-clidependencyreact-i18nextto frameworks list and configurationuseTranslatescopes which is react specific fromi18nexttoreact-i18nextt('key', { ns: 'namespace' })and<Trans i18nKey="key" ns="namespace" />The changes have also been tested on my project and I can confirm it is working. However, another set of eyes would be appreciated so that we don't have a repeat of #735.
Resolves #677, resolves #688, resolves #823, resolves #786