Skip to content

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

herkulano
Copy link

Closes #7344

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

@herkulano herkulano force-pushed the fix-uselocale-causes-additional-re-render branch 2 times, most recently from 80ebc13 to 6facd6f Compare July 10, 2025 17:47
@@ -77,14 +75,5 @@ export function useDefaultLocale(): Locale {
};
}, []);

// We cannot determine the browser's language on the server, so default to
Copy link
Member

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?

Copy link
Author

@herkulano herkulano Jul 11, 2025

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.

Copy link
Author

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.

Copy link
Author

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.

@herkulano herkulano force-pushed the fix-uselocale-causes-additional-re-render branch from ca550f2 to e8af64e Compare July 11, 2025 10:07
@herkulano herkulano force-pushed the fix-uselocale-causes-additional-re-render branch from e8af64e to e5572e1 Compare July 11, 2025 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

useLocale usage causes additional re-render
2 participants