-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix: remove setting defaults in ssr from useDefaultLocale #8524
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
base: main
Are you sure you want to change the base?
fix: remove setting defaults in ssr from useDefaultLocale #8524
Conversation
80ebc13
to
6facd6f
Compare
@@ -77,14 +75,5 @@ export function useDefaultLocale(): Locale { | |||
}; | |||
}, []); | |||
|
|||
// We cannot determine the browser's language on the server, so default to |
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.
So I don't think we want to completely remove this. Instead, we want to split the I18nProvider's internals into two components, one which calls the useDefaultLocale and one which doesn't. This is how to conditionally call hooks since they can't be in an if statement.
Which component we call inside the I18nProvider should be determined by if the locale
prop is passed to the I18nProvider.
see proposal here: #7344 (comment)
Side note, if you've done it this way instead intentionally, can you explain your decision?
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, made the changes to split the provider.
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.
Although the currentLocale
is set to the default en-US
even on SSR:
let locale = typeof window !== 'undefined' && window[localeSymbol]
// @ts-ignore
|| (typeof navigator !== 'undefined' && (navigator.language || navigator.userLanguage))
|| 'en-US';
So setting a default on SSR in the useDefaultLocale
feels unnecessary.
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.
After trying the provider split I realized that we need to re-render on languagechange
and removing the useDefaultLocale()
hook entirely from the provider breaks components like NumberField that require a re-render when changing language: https://app.circleci.com/pipelines/github/adobe/react-spectrum/27484/workflows/dad88c6d-a527-49a0-bad5-02a412732311/jobs/470813
I now believe the initial approach was correct and we don't need to split the provider as the useDefaultLocale()
is necessary even if the locale is set in the provider. I believe the extra re-render comes from returning the default locale on SSR.
ca550f2
to
e8af64e
Compare
A component uses`useDefaultLocale` and another the locale value set in the Provider.
e8af64e
to
e5572e1
Compare
Closes #7344
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: