Fix/bug 1691 preserving change language binding

[timheilman]

Checklist

• only relevant code is changed (make a diff before you submit the PR)
• run tests npm run test
• tests are included
• commit message and code follows the Developer's Certification of Origin

Checklist (for documentation change)

• only relevant documentation part is changed (make a diff before you submit the PR)
• motivation/reason is provided
• commit message and code follows the Developer's Certification of Origin

[coveralls]

coverage: 97.029% (+0.04%) from 96.986%
when pulling 3c3f512 on timheilman:fix/bug-1691-preserving-changeLanguage-binding
into ac3f71c on i18next:master.

[timheilman]

Sorry about the initial bad fix in #1716 . A dev issue was uncovered over at #1716 (comment) . Unfortunately I still cannot figure out how to test the bug 1691 proper in a unit test, but the dev issue does have a test added that:

1. succeeds before Fix/bug 1691 make returned t function identical upon second effect run in strict mode #1716
2. fails after Fix/bug 1691 make returned t function identical upon second effect run in strict mode #1716 , demonstrating the dev issue
3. succeeds after this PR

I also have tested this PR manually for both bug 1691 proper as well as for the dev issue. I welcome @adrai and @medihack 's eyes on this second attempt, as well as anyone else's interested. No time pressure.

[adrai]

thank you... it's included in v14.0.5

[adrai]

fyi: verified to work also in this example 8334298 👍

[schontz]

I know this is already merged, but the side-effect of this approach is that you define a new t for every single component, instead of re-using t unless the props actually change. I understand that useTranslation takes arguments that can affect that, but that slows things down for the "happy path" of useTranslation() with no arguments quite a bit. I'd suggest memoizing that version globally.

[timheilman]

I know this is already merged, but the side-effect of this approach is that you define a new t for every single component, instead of re-using t unless the props actually change. I understand that useTranslation takes arguments that can affect that, but that slows things down for the "happy path" of useTranslation() with no arguments quite a bit. I'd suggest memoizing that version globally.

Thank you @schontz for the suggestion. It seems #1756 may be related to what you are seeing here. I plan to look into that issue more closely later today.

Unfortunately, I cannot see what you are describing in your comment on this PR, 1720. What I am seeing in src/useTranslation.js is that getNewT, post-change line 80, should be an identical implementation to the pre-change getT, pre-change line 59. All places other than post-change line 142 that had been using getT should be getting the same implementation post-change from getNewT at post-change lines 104, 108, 114, and 118. Only at post-change line 142 should the implementation have changed: to newly-memoize the t function.

However, my line of reasoning does not align with this research from @arvinxx that version 14.0.4 does have a performance issue, so my line of reasoning is not correct. I'm sharing my reasoning in case you can see the error in it.

[timheilman]

If 14.0.1 exhibits the bug #1756 , then the fix to #1756 in 14.0.2 comes at the cost of causing bug #1718 , which is why the change in 14.0.2 was rolled-back in 14.0.3. Please see my recent comment on issue 1756.

If that is the case, then I would ask @schontz to provide a minimal reproduction with expected and actual results, similar to the way that @woodreamz did in #1691 . That "To Reproduce" section with its behavior that reliably differs from the "Expected behavior" is the gold standard in bug reports. If you can file an issue that way, I've been very impressed with @adrai 's responsiveness to PRs provided by the community, and your concern has a good chance to be addressed.