Skip to content
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

Fix/bug 1691 make returned t function identical upon second effect run in strict mode #1716

Conversation

timheilman
Copy link
Contributor

@timheilman timheilman commented Feb 1, 2024

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

@adrai
Copy link
Member

adrai commented Feb 1, 2024

@medihack can you review this?

@coveralls
Copy link

Coverage Status

coverage: 97.027% (+0.04%) from 96.984%
when pulling 76d0d11 on timheilman:fix/bug-1691-make-returned-t-function-identical-upon-second-effect-run-in-strict-mode
into 0321c04 on i18next:master.

@timheilman
Copy link
Contributor Author

testing-library/react-testing-library#1241 was just merged, but I haven't looked closely yet at it to see if hook-based testing support was added for useEffect double-run in strictMode

@timheilman
Copy link
Contributor Author

OK, I went down a bit of a rabbit hole toward writing a unit test for this, but it looks like strictMode is integrated into @testing-library/react (which subsumed @testing-library/react-hooks) only for v18 of react and higher, whereas react-i18next is still supporting v16 of react. That precludes using the strictMode testing configuration, without which it's hard for me to see how to test this change otherwise than as described in the "To Reproduce" section of the issue. That reproduction is reliable and is indeed fixed by this change.

@timheilman timheilman marked this pull request as ready for review February 1, 2024 19:23
@adrai
Copy link
Member

adrai commented Feb 1, 2024

ok, no test required, but I like @medihack to check if this change will influence in some way the issue he had here

@adrai adrai linked an issue Feb 1, 2024 that may be closed by this pull request
@adrai
Copy link
Member

adrai commented Feb 1, 2024

//cc: @kachkaev

@timheilman
Copy link
Contributor Author

ok, no test required, but I like @medihack to check if this change will influence in some way the issue he had here

I totally understand. This is a widely-depended-upon library that I'm certainly no expert with. I welcome any/all code review comments and/or edits by those more familiar with the code.

The test @medihack added alongside the change for the issue of changing the provider-provided i18n instance is still passing, which is a good sign. This makes sense since the useCallback memoization does have that i18n instance in its dependency list, and so should generate a new fixedT. However, the truth is that I'm not very familiar at all with this code, and it is hairy, so I still very much welcome more experienced eyes on it.

@medihack
Copy link
Contributor

medihack commented Feb 1, 2024

At first glance, it looks good. If there is no time pressure, I would take a deeper look and try it out on the weekend if that is ok for you.

@adrai
Copy link
Member

adrai commented Feb 1, 2024

wonderful... thank you @medihack

@medihack
Copy link
Contributor

medihack commented Feb 3, 2024

The PR looks good to me. I also gave it a quick test drive and got no problems (with shallow routing in Next.js).

@adrai adrai merged commit f8a4e19 into i18next:master Feb 4, 2024
6 checks passed
@adrai
Copy link
Member

adrai commented Feb 4, 2024

Thank you very much for your contributions... it's included in v14.0.2

@jaredhan418
Copy link

Why!!!!!! Why!!!!! Damn!
i18n.changeLanguage will never work immediately in every env!
StrictMode is so damn bad!

@adrai
Copy link
Member

adrai commented Feb 5, 2024

@jaredhan418 is right... this PR breaks the language change rendering...
to reproduce update this example and try to change the language: https://github.com/i18next/react-i18next/tree/master/example/react

@timheilman can you have a look at it? @medihack can you assist?

adrai added a commit that referenced this pull request Feb 5, 2024
@adrai
Copy link
Member

adrai commented Feb 5, 2024

I temporarily reverted it with v14.0.3

@medihack
Copy link
Contributor

medihack commented Feb 6, 2024

That's unfortunate :-( And unfortunately, I don't have enough time right now. I'll try my best in the upcoming weeks, but it looks like a nasty issue to fix entirely.

@timheilman
Copy link
Contributor Author

happy to take a look! Though of course I can't promise anything. Thanks @jaredhan418 for your testing and notifying of the breakage, and thanks @adrai for being so on-top of this repo. Reversion is obviously the right move.

timheilman added a commit to timheilman/react-i18next that referenced this pull request Feb 6, 2024
…f fixedT function,

bad impl, breaks test that was introduced in parent commit, verifying bug here: i18next#1716 (comment)
adrai pushed a commit that referenced this pull request Feb 6, 2024
* test: bug 1691 second attempt, add test for actual use of i18n.changeLanguage with useTranslation

* fix: (pre-cleanup) bug 1691 cherry pick of 168b8f0: use memoization of fixedT function,

bad impl, breaks test that was introduced in parent commit, verifying bug here: #1716 (comment)

* fix: (pre-cleanup) bug 1691 fixing dev issue: regenerate a new t fn everywhere

other than where the bug was occurring, and keep shallow routing working via dependency list of useCallback hook.  Fixes test broken by parent commit.

* style: cleanup debug messages for bug 1691

* build: bug 1691 npm run build
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

t function from useTranslation is unstable in concurrent mode
5 participants