-
Notifications
You must be signed in to change notification settings - Fork 152
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: Fix SSR support in I18nProvider #861
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #861 +/- ##
=======================================
Coverage 91.86% 91.86%
=======================================
Files 582 582
Lines 14846 14849 +3
Branches 4758 4761 +3
=======================================
+ Hits 13638 13641 +3
Misses 1126 1126
Partials 82 82
... and 1 file with indirect coverage changes 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. |
// and lastly default to English. Locales have a recommended case, but are | ||
// matched case-insensitively, so we lowercase it internally. | ||
const locale = (providedLocale || document?.documentElement.lang || 'en').toLowerCase(); | ||
let locale: string; |
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.
Are you still sure you want to go your own hard way instead of reusing the existing code?
https://github.com/cloudscape-design/components/blob/main/src/internal/utils/locale/normalize-locale.ts
If not reusing, why not?
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.
I'm a little cautious of partially using the file, because the date component logic is wrong for this.
I mentioned in the earlier PR that I think the best course of action would be to make this the definitive source of truth for internationalization and have the date components follow the provider instead. But you're right, maybe I can rename On further thought... (see below)normalizeLocale
to normalizeLocaleForDateComponents
and have a nice global locale file for everything.
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.
To expand further: there would be very little shared code - just two similar use cases bunched up into a single file just making things confusing to understand. Philosophically, it's more important for the locales to be used to match the correct strings in the messages JSON than to be valid or accurate BCP47 locale strings.
The context:
- shouldn't "fix" underscore delimited locale strings (because they're used for key accesses)
- shouldn't "auto-expand" the provided locale with the current browser locale or html locale
- shouldn't fall back to the browser locale (cause that makes SSR mismatches even worse)
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.
For posterity: this rabbit hole is deeper than first expected. Writing a proposal for that, but in the mean time, there's very little opportunity for code sharing. Merging this fix for now.
ec50f5c
to
9cdb044
Compare
Description
Whoops. This one's on me. Thought optional chaining on
document
would save me here, but I would still need to explicitly check that the variable is even defined in SSR.Related links, issue #, if available: n/a
How has this been tested?
Wrote an SSR test to make sure this doesn't break.
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md
.CONTRIBUTING.md
.Security
checkSafeUrl
function.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.