-
Notifications
You must be signed in to change notification settings - Fork 225
Update and Map Deprecated Timezone #2842
Conversation
|
🔎 View this PR in Shipit Next. ℹ️ Expand to learn how to deploy and handle emergencies using Shipit NextOverviewShipit Next will merge your code on your behalf because this repository uses Shipit Next and its merge queue. To ship this PR, you can either:
Comment Commands
Commands exclusive to Deploy Before Merge
Documentation
Questions or feedback?
|
|
/snapit |
packages/react-i18n/src/i18n.ts
Outdated
| } | ||
|
|
||
| private normalizeTimezone(timeZone?: string) { | ||
| return timeZone ? mapDeprecatedTimezones(timeZone) : undefined; |
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.
We initially had a test added to this - but the tests were actually a false positive, testing date for date, the test for the mapping is actually seen in
| it('maps deprecated timezones', () => { |
Co-authored-by: Trish Rempel <[email protected]> Co-authored-by: Christina Tran <[email protected]> Co-authored-by: Jess Telford <[email protected]>
|
/snapit |
|
🫰✨ Thanks @ginabak! Your snapshots have been published to npm. Test the snapshots by updating your "@shopify/dates": "0.0.0-snapshot-20250108014630",
"@shopify/react-i18n": "0.0.0-snapshot-20250108014630",
"@shopify/react-i18n-universal-provider": "0.0.0-snapshot-20250108014630" |
Co-authored-by: Trish Rempel <[email protected]> Co-authored-by: Christina Tran <[email protected]>
|
/snapit |
| return memoizedGetDateTimeFormat(locales, { | ||
| ...options, | ||
| timeZone: options.timeZone | ||
| ? mapDeprecatedTimezones(options.timeZone) |
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.
HIII @jesstelford ❤️ SOOOOOOOOOO when we implemented the change inside i18n and tested out the snapshots ... we weren't able to see the changes actually fix the issue 😬 .... so we're trying it out directly in formatDate as parts of web directly call formatDate from @shopify/dates we shipped https://github.com/Shopify/web/pull/152962/files that actually fixes it in web, but if we wanted to do our "due diligence" for other repo's, thought we might as well try this LOL 😄
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.
Interesting! During your testing, did you notice any regressions with this change (trying some different mapped and non-mapped timezones)? If not, I agree we should ship this as a bug fix to ensure we've covered all our bases.
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.
HIII JESS!!! do you mean for the i18n testing? Or this current one? SADLY Npm hasn't finished propogating lol, so I'm just waiting to test 😢
|
🫰✨ Thanks @ginabak! Your snapshots have been published to npm. Test the snapshots by updating your "@shopify/dates": "0.0.0-snapshot-20250108211454",
"@shopify/react-i18n": "0.0.0-snapshot-20250108211454",
"@shopify/react-i18n-universal-provider": "0.0.0-snapshot-20250108211454" |
| }; | ||
|
|
||
| formatDate(date, locale, options); | ||
| expect(mapDeprecatedTimezones).toHaveBeenCalledWith('Europe/Kyiv'); |
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.
THIS TEST ISN'T GREAT ... but like at least we're testing that the function is actually being called, AND the fact that quilt/packages/dates/src/tests/map-deprecated-timezones.test.ts is testing that it's mapping / converting, feels safe to me - Ideally mocking out a real "bug" would be great here, but since we can't ... this is good enough imo 😄 ❤️

Description
(similar to
shopify-internal)📣 ❤️ co-authors with @trishrempel @christina-tran @jesstelford ❤️
Preps https://github.com/Shopify/web/issues/146409
Preps https://github.com/Shopify/web/issues/150625
There are some bugsnag errors for the error,
errorMessage:"Invalid time zone specified: Europe/Kyiv", although browser versions are part of this 🙄 , we decided that the best fix would be to allow for deprecated timezones as some of the browser versions are recent like131for chrome, seen here on BugSnagThis PR:
Europe/Kyivfor older browsers 😬 (basically it's now backwards compatible), a thought though ... I'm curious if the deprecatedEurope/Kievis no longer allowed (which probably won't happen for a while ... ) we'll have to delete this line 😄Europe/Kyivtimezone for react-i18n