-
Notifications
You must be signed in to change notification settings - Fork 382
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
refactor: use useSyncExternalStore to subscribe for i18n updates #1746
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
size-limit report 📦
|
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1746 +/- ##
==========================================
+ Coverage 75.95% 75.98% +0.02%
==========================================
Files 80 80
Lines 2063 2065 +2
Branches 529 530 +1
==========================================
+ Hits 1567 1569 +2
Misses 383 383
Partials 113 113
☔ View full report in Codecov by Sentry. |
don't have a react competency to review this one. If you think this would work better, i'll trust you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vonovak thank you!
@vonovak this change increased size of react package from 2 kB to 2.27 kB due to some issue in the workflows it wasn't caught directly in this PR. Could you increase a size limit budget, or re-consider the need of this change. |
hi @thekip, I therefore don't see an issue with increasing the size budget at this point. With regard for the need for this change, it's true that this change is not in response to some bug reported or something like that, it's more of a pre-emptive change with not-so-clear-necessity. I'll try to get another opinion on this diff. |
@vonovak got your point, just increase bundle budget, please |
can you send me a link to a pipeline that's failing or some more info about this? If you can't change the bundle budget, I'm not sure I'll be able to do it, I only have read rights here... Thanks :) |
…pdates (lingui#1746)" This reverts commit 47d4fdb.
Description
this refactors the change subscription mechanism used in Lingui React Provider to use the
useSyncExternalStore
hook, which according to this offers better support for "concurrent rendering features like selective hydration and time slicing". That being said, I'm not sure how exactly this should be better. Maybe someone will chime in and add more insight.Everything should work the same except:
Previously, when the Provider was re-rendered with different i18n instance of different
defaultComponent
, it would ignore them. This likely never was an issue, but technically speaking, it was a bug. Now that is fixed.What I removed is detection of the cases when i18n.change would happen (catalog update, locale update) before the Provider subscribed for the change event. I removed the detection and a related test case, because I think it's a very unlikely scenario, but I can put it back if desired.
Types of changes
Fixes # (issue)
Checklist