-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix: Normalize literal before dayPeriod in formatToParts #8826
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: Normalize literal before dayPeriod in formatToParts #8826
Conversation
26832a4
to
e1422b8
Compare
e1422b8
to
cc5ac1e
Compare
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.
Thanks for starting the PR, I think we have some more decisions to make.
@@ -42,6 +42,17 @@ describe('DateFormatter', function () { | |||
]); | |||
}); | |||
|
|||
it('should format to parts with space as literal before am/pm', function () { | |||
let formatter = new DateFormatter('en-US', {timeStyle: 'short'}); |
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.
TL;DR
I'm a little worried we're going to be playing whack-a-mole with this. I think it'd be better if we could somehow allow suppressHydrationWarning
through or hold off rendering for a cycle on server and client when SSR.
Now on to findings:
Many countries prefer the non-breaking space https://www.stylemanual.gov.au/grammar-punctuation-and-conventions/numbers-and-measurements/dates-and-time
Across our supported locales:
In Chrome, it would appear that the non-breaking space char 8239 is the more commonly used.
In Safari, it's overwhelmingly char 32, except for ar-AE
which uses char 160
.
In Firefox, it's only char 32.
So I think we are ok to default it to 32 for now. But it looks like more than just Node is going to need a fix.
I got this from running the script provided in the issue
['ar-AE', 'bg-BG', 'cs-CZ', 'da-DK',
'de-DE', 'el-GR', 'en-US', 'es-ES',
'et-EE', 'fi-FI', 'fr-FR', 'he-IL',
'hr-HR', 'it-IT', 'ja-JP', 'ko-KR',
'lt-LT', 'lv-LV', 'nb-NO', 'nl-NL',
'pl-PL', 'pt-BR', 'pt-PT', 'ro-RO',
'ru-RU', 'sk-SK', 'sl-SI', 'sr-SP',
'sv-SE', 'tr-TR', 'uk-UA', 'zh-CN',
'zh-TW'].map((locale) => {
const formatter = new Intl.DateTimeFormat(locale, { timeStyle: 'long', hour12: true });
const segments = formatter.formatToParts(new Date())
const secondsSegmentIndex = segments.findIndex(segment => segment.type === 'dayPeriod');
if (secondsSegmentIndex > 0) {
return segments[secondsSegmentIndex-1].value.charCodeAt(0);
}
return 'not used'
})
This should be tested across every locale we support. I also noticed that the test uses timeStyle short, but the example script used long. We would also need to use hour12: true
in the options since many locales default to a 24hr representation.
Something along these lines with the right assertions (not just a non-null check).
it.each(['ar-AE', 'bg-BG', 'cs-CZ', 'da-DK',
'de-DE', 'el-GR', 'en-US', 'es-ES',
'et-EE', 'fi-FI', 'fr-FR', 'he-IL',
'hr-HR', 'it-IT', 'ja-JP', 'ko-KR',
'lt-LT', 'lv-LV', 'nb-NO', 'nl-NL',
'pl-PL', 'pt-BR', 'pt-PT', 'ro-RO',
'ru-RU', 'sk-SK', 'sl-SI', 'sr-SP',
'sv-SE', 'tr-TR', 'uk-UA', 'zh-CN',
'zh-TW'])('should format to parts with space as literal before am/pm', function (locale) {
let formatter = new DateFormatter(locale, {timeStyle: 'short', hour12: true});
console.log(locale, formatter.formatToParts(new Date(2020, 1, 3, 13, 0)));
expect(formatter.formatToParts(new Date(2020, 1, 3, 13, 0))).not.toBeNull();
let formatter2 = new DateFormatter(locale, {timeStyle: 'long', hour12: true});
console.log(locale, formatter2.formatToParts(new Date(2020, 1, 3, 13, 0)));
expect(formatter2.formatToParts(new Date(2020, 1, 3, 13, 0))).not.toBeNull();
let formatter3 = new DateFormatter(locale, {dateStyle: 'full', timeStyle: 'long', hour12: true});
console.log(locale, formatter3.formatToParts(new Date(2020, 1, 3, 13, 0)));
expect(formatter3.formatToParts(new Date(2020, 1, 3, 13, 0))).not.toBeNull();
});
Good news, it doesn't crash in locales such as zh-TW
which actually places the dayPeriod
first
zh-TW [
{ type: 'dayPeriod', value: '下午' },
{ type: 'hour', value: '1' },
{ type: 'literal', value: ':' },
{ type: 'minute', value: '00' },
{ type: 'literal', value: ':' },
{ type: 'second', value: '00' },
{ type: 'literal', value: ' [' },
{ type: 'timeZoneName', value: 'GMT+11' },
{ type: 'literal', value: ']' }
]
However, while testing, I ran into an issue ines-ES
where the space is inside the dayPeriod itself in addition to outside. In Node, Chrome, and Firefox it's 160, and in Safari it's 32
const formatter = new Intl.DateTimeFormat('es-ES', { timeStyle: 'long', hour12: true });
const segments = formatter.formatToParts(new Date())
console.log(segments[6].value.charCodeAt(2));
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.
Sorry I couldn't follow up earlier. I spent some time looking at formatToParts
outputs in each of the locales you used in your examples and I found that the rabbit hole goes way deeper.
One finding of mine that really puts me off pursuing this further is that it's not just the white space before the dayPeriod
that might cause hydration issues. I identified at least 6 other mismatches between browser and Node outputs:
- in Chrome for
nl-NL
you get01:00:05 p.m. Eastern Standard Time
compared to Node's01:00:05 p.m. Eastern-standaardtijd
- in Chrome for
ko-KR
you get오후 1시 0분 5초 북미 동부 표준시
compared to Node's오후 1시 0분 5초 미 동부 표준시
(there is one extra character there in Chrome) - in Firefox more than half of locales omit the leading zero in the hour segment
01:00:05
vs1:00:05
- in Firefox and Safari
he-IL
output is missing some characters at the end - in Safari
el-GR
usesμμ
for PM compared to Node'sμ.μ.
.lv-LV
,nb-NO
,nl-NL
,pt-PT
also use mismatching vocabulary for day periods - in Safari
zh-TW
prints東部標準時間 下午1:00:05
compared to Node's下午1:00:05 [東部標準時間]
I'd say that, if it was just one white-space character that had to be normalized across locales, that would've been feasible to do here in @internationalized/date
, but, seeing that there is no pattern to this madness and any sort of normalization would require us to make decisions on behalf of users we don't speak the languages of, I believe closing this PR is the only sensible thing one can do.
In my company, I would advocate that we wrap the affected input components in a dynamic import like this https://nextjs.org/docs/messages/react-hydration-error#solution-2-disabling-ssr-on-specific-components. It's not clean and will result in layout shifts, but that should to be good enough for our use case.
I don't think the library should do anything to skip render cycles or not render on the server. I think consumers are the ones that should decide if they want to do that as it might affect the user experience or what javascript-less web crawlers see on the page.
As for suppressHydrationWarning
, you could pass that through. You might even just set it to true yourself unconditionally, considering how common this issue might be. However, with my findings in mind, you might need to add that to not just LiteralSegment
, but also EditableSegment
.
Closes #7169
Node's and Chromium's
formatToParts
returnNARROW NO-BREAK SPACE
for the literal beforedayPeriod
. This causes hydration errors in Firefox and Safari, which use regular space for the same literal.Since
format
returns regular space, it makes sense to standardizeformatToParts
to also use regular space.I would've liked to refer to an issue in v8, but there was already one open and they claimed it's in-actionable for them. nodejs/node#49222
At present, it's not clear when Chromium and Node (if ever) are going to fix the inconsistency between
formatToParts
andformat
.✅ Pull Request Checklist:
📝 Test Instructions: