Fix language switching and update date-fns to v4 #2788
Conversation
- `hasOwnProperty` does not work with i18n.languages, as it's an array. It is also empty on init. - Create function to find first supported locale among devices locales. (I experimented with passing multiple languages to i18next with a language detector, but its fallback language is not ideal and will just fall back to the global `fallbackLng` instead of trying the second option in the list.) - Do a little more to ensure we only set RTL to true if the locale we end up picking is RTL, although it's still not perfectly guaranteed here, as i18next could fall back to `en` for some reason. The problem is that we either need to set RTL immediately, or restart the app if RTL setting needs to change.
- Fix imports - Conditionally require locales - Add more locale support
| @@ -27,14 +27,14 @@ const episode = EpisodeModel.create(data) | |||
| test("publish date format", () => { | |||
| expect(episode.datePublished.textLabel).toBe("Jan 20, 2022") | |||
| expect(episode.datePublished.accessibilityLabel).toBe( | |||
| 'demoPodcastListScreen:accessibility.publishLabel {"date":"Jan 20, 2022"}', | |||
| "demoPodcastListScreen:accessibility.publishLabel", | |||
There was a problem hiding this comment.
Can you explain please why are we removing the date? Reading from the PR description, it seems like we still want to improve this test later.
There was a problem hiding this comment.
I don't really understand why this changed, but I also didn't want to block this on this test, because the behavior is correct when I actually test it in the app.
There was a problem hiding this comment.
The single quotes changing to double quotes was required by the linter, and I think might be related to tests having a separate TS config? but not sure, I thought that would be handled by prettier.
Actually I'm even more confused by this because apparently it failed on the CLI project's prettier check (https://app.circleci.com/pipelines/github/infinitered/ignite/3631/workflows/6e7e16e3-e4c1-4a6b-949b-d1fe26da1665/jobs/4375), but the inner project isn't complaining about the double-quotes.
That config just isn't unified, so we get weird things like this sometimes.
There was a problem hiding this comment.
My question was not related to quotes but related to deleting the date within it.
There was a problem hiding this comment.
Yes, for that part see my first comment. I'm not sure why it changed, but I'd also like to not block this PR on that since the logic is working correctly on the real app, this is just a test that I'm not sure is testing anything useful the way it's currently set up.
There was a problem hiding this comment.
Filed issue for this: #2789
| const systemTagMatchesSupportedTags = (deviceTag: string) => { | ||
| const primaryTag = deviceTag.split("-")[0] | ||
| return supportedTags.includes(primaryTag) | ||
| } |
There was a problem hiding this comment.
What it be worth writing a test for this one?
There was a problem hiding this comment.
I was thinking splitting on - and checking includes were simple enough it didn't need a test, if we did something more complex maybe then?
Most of the complexity here comes in with third party systems, so an E2E test to check that we can switch languages makes the most sense to me.
There was a problem hiding this comment.
Yeah, I agree this is not complex, but I'm also thinking of future Felipe doing an i18 migration again. It's all good; not a blocker.
There was a problem hiding this comment.
For sure. I still think in that case what is most useful is an E2E test instead of unit tests.
| ) | ||
| }) | ||
|
|
||
| test("duration format", () => { | ||
| expect(episode.duration.textLabel).toBe("42:58") | ||
| expect(episode.duration.accessibilityLabel).toBe( | ||
| 'demoPodcastListScreen:accessibility.durationLabel {"hours":0,"minutes":42,"seconds":58}', | ||
| "demoPodcastListScreen:accessibility.durationLabel", |
|
🎉 This PR is included in version 10.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Please verify the following:
yarn testjest tests pass with new tests, if relevantyarn linteslint checks pass with new code, if relevantyarn format:checkprettier checks pass with new code, if relevantREADME.md(or relevant documentation) has been updated with your changesDescribe your PR
While updating date-fns to v4, I ran into a few issues with language switching.
Languages were not being loaded correctly
The previous way we had of loading languages with i18n-js doesn't work with i18next.
hasOwnPropertydoes not work withi18n.languages, as it's an array.i18n.languagesis also empty on init, as it's only an array of the currently configured languages to use (preferred, then fallback), not all supported languages.Instead I created a function to pick the first locale from device locales that we support, and configure i18n with that.
Fast refresh would crash the app
This we fix by referring to i18n from the library, we don't need to store it in a variable in
i18n.ts.Updates for date-fns v4
Here I just needed to update some import paths, but while I was there, I also added support for more of our supported locales, and conditionally load only the locale actually in use.
Future improvements
I recommend we decide whether and how we want to follow up on these items later:
Episodetest. It's not clear what this test wants to assert. If it wants to assert that the models are accounting for params correctly, we may have to find a way to manually test that, probably spying ont(). If it wants to assert that we're translating accessibility labels correctly, we should make it an E2E test. Etc.translatefunction everywhere which doesn't know to react when language is changed (unlike if we were using theuseTranslationhook.)