fix: format should follow locale order #1619
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 @@
## next #1619 +/- ##
==========================================
+ Coverage 75.53% 75.55% +0.01%
==========================================
Files 77 77
Lines 1991 1988 -3
Branches 520 517 -3
==========================================
- Hits 1504 1502 -2
+ Misses 374 373 -1
Partials 113 113
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
| expect(date(["en"], new Date(2023, 2, 5))).toMatchInlineSnapshot( | ||
| `"3/5/2023"` | ||
| ) | ||
| expect(date(["en"], new Date(2023, 2, 5))).toBe("3/5/2023") |
There was a problem hiding this comment.
it might be intended to use toMatchInlineSnapshot to make sure string format is generated. But I will leave it up to @thekip
There was a problem hiding this comment.
if you compare x with "3/5/2023" using toBe, then it's immediately clear that x is a string, so I don't see that as an issue 🙂
There was a problem hiding this comment.
it's just easier to write tests from scratch with toMatchInlineSnapshot. Here when @vonovak refactor tests you already have an expected result. When i wrote them, expected result had to be figured out, so with toMatchInlineSnaphsot is just more convenient, no more difference here
| locales?: readonly string[], | ||
| options: unknown = {} | ||
| locales: readonly string[], | ||
| options?: Intl.DateTimeFormatOptions | Intl.NumberFormatOptions |
There was a problem hiding this comment.
maybe default to {} as before, the differences are:
JSON.stringify(undefined)
undefined
JSON.stringify({})
'{}'
so it affects the generated cache key, unless you intend to make cacheKey(type, locales) to generate a different key compared to cacheKey(type, locales, {}) ?
There was a problem hiding this comment.
yes, I can revert that part
unless you intend to make cacheKey(type, locales) to generate a different key compared to cacheKey(type, locales, {})
no, that is not an intent but a side effect. I thought, "how are people going to call this"? And I think it's unlikely they would call i18n. number(123, {}). However, if they do call i18n. number(123, {}) and i18n. number(123}) then it's true that the cache would have 2 entries for the same number formatter which is not great but I also thought about the default parameter options = {} which means that every time i18n. number(123}) is called there is an empty object allocated and disposed. It's not important though, I can revert this part...
|
Kinda very corner case, but it's good to be caught and fixed. Just out of curiosity, have you really encountered this issue on the real project or just spotted the issue looking in the codebase? |
just randomly spotted it :) |
|
yes |
Description
fixes a bug in formatting:
cacheKeywould sort locales to obtain cache key; however, the order of passed locales matters.The tests that I added would previously fail
Types of changes
Fixes # (issue)
Checklist