-
Notifications
You must be signed in to change notification settings - Fork 16
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: use i18next-scanner v3 for better i18next compatibility #864
Conversation
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.
Well that's crummy 🫠
I approve as I think this is the least disturbing for people (prevent crash on running yarn start
)
- I guess we don't really know if i18next-scanner v3 is also making use of some breaking feature in certain i18next versions, so if one of our apps has a very old install of cli-app-scripts/old i18next-scanner, I guess we can run into this issue again 🙃 (maybe we should ask i18next-scanner team about this
*
dependency?) - it's a bit disappointing that we're falling back to i18next-scanner v3 when we know v4 was release because of a bug 🫤. I guess the specific bug isn't really relevant to how we do translations, but slightly annoying.
- I agree that we should update the i18next dependency in d2-i18n, though also think that cli-app-scripts>i18next-scanner dependency will not pick that update up, and it seems we can't resolve to common i18next version as we don't use d2-i18n in cli? It does seem that even though we're really behind in 18next version in d2-i18n that it's mostly functionality we don't use, so hopefully it wont be a problem 🤞
For what it's worth simply upgrading @dhis2/cli-app-scripts does not work for me anyway as I also have to upgrade the @dhis2/app-runtime package to get the app to work (and also remove the existing shell). I usually just remove/reinstall cli-app-scripts as I find that it's the easiest to make sure dependencies update properly, but maybe that's a bit of an antipattern to suggest that 😄
## [11.6.4](v11.6.3...v11.6.4) (2024-07-19) ### Bug Fixes * use i18next-scanner v3 for better i18next compatibility ([#864](#864)) ([84a5a59](84a5a59))
🎉 This PR is included in version 11.6.4 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Thanks for the thoughtful feedback! 🙏 Yeah I agree this isn't ideal, for those reasons you described 🙃 For upgrading yarn add -D @dhis2/cli-app-scripts
npx yarn-deduplicate yarn.lock && yarn Here's also some notes I have about if adding/updating our own I’m kinda unfamiliar with some of the details of how dependencies get resolved… like, the example I was wondering about above, if you have this situation:
Then what happens in... Scenario 1: Data Visualizer w/
Scenario 2: Dashboard starting w/
And unfortunately, it’s not a convenient thing to test, since in an app, you can’t really modify a dependency’s Right now, something I could investigate in the Dashboard: it currently has a root dependency on
I say “somehow”, because when I uninstall and reinstall
…and plurals break again
So maybe setting |
# [12.0.0-alpha.4](v12.0.0-alpha.3...v12.0.0-alpha.4) (2024-07-23) ### Bug Fixes * use i18next-scanner v3 for better i18next compatibility ([#864](#864)) ([84a5a59](84a5a59)) * **deps:** update i18next-scanner version to support old plurals format again ([#861](#861)) ([d0e433b](d0e433b)) * plugin boundary retry if plugin logic is skipped ([#862](#862)) ([01a3160](01a3160)) ### Features * update boilerplate app code for init command [LIBS-644] ([#866](#866)) ([bd6cfc0](bd6cfc0))
🎉 This PR is included in version 12.0.0-alpha.4 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Follow-up on an issue introduced by LIBS-641
The problem I encountered:
When upgrading
@dhis2/cli-app-scripts
to v11.6.3 in the Data Visualizer app, i18n extraction broke with an error likei18nextInstance.services.pluralResolver.getSuffixes is not a function
The cause:
i18next-scanner
introduced the use of this API, which is only available ini18next
v20+ (although this change was not really documented, and because ofi18next-scanner
's use of"*"
for the version on theiri18next
dependency, maybe they're not even aware of it)i18next-scanner
's use of"*"
for the version on theiri18next
dependency continues to prove problematic:i18next-scanner
v2, when an app installed a new version of@dhis2/cli-app-scripts
, it installed the latest version ofi18next
(v23), whichi18next-scanner
couldn't handle — this broke how plurals were parsed when extracting i18n strings@dhis2/cli-app-scripts
, thei18next
version is left alone, which caused the issue described at the topi18next
v19, and upgrading@dhis2/cli-app-scripts
doesn’t affect iti18next-scanner
v4 tried to use thei18next
v20 API, it brokePossible solutions:
i18next
in our libraries to make sure its >v20, but I'm not sure this would work... i.e., would i18next-scanner keep using whatever i18next version was installed when@dhis2/cli-app-scripts
was first installed, and we end up with one more version ofi18next
in the dependencies?cli-app-scripts
>i18next-scanner
(used for build-time string extraction) &d2-i18n
(used for runtime translation)i18next-scanner
>i18next
@ *d2-i18n
>i18next
@ ^10d2-i18n
13 major versions should take some consideration, I thinki18next-scanner
to v3 in the CLI here: [This is my proposal, and what I did in this PR]i18next-scanner
andi18next
can be upgraded when we make the full effort to support the new translation format