Skip to content
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

Accept an array as localesPath in useLocaleRequired, withLocaleRequired, and LocaleRequired #779

Merged

Conversation

eliebman-godaddy
Copy link
Contributor

Summary

Support passing an array of localePaths to useLocaleRequired, withLocaleRequired, and LocaleRequired
This helps optimize load times by fetching paths in parallel. Previously, consumers had to nest withLocaleRequired HOCs in order to ensure fetching of multiple paths, which caused requests to run in series, creating longer page load times.

Addresses Issue #776

Changelog

  • Accept an array as localesPath in useLocaleRequired, withLocaleRequired, and LocaleRequired

Test Plan

Unit Tests are passing
Have not been able to verify inside an application due to node versioning issues, but we should probably verify this functionality with npm link before merging.

@eliebman-godaddy eliebman-godaddy requested a review from a team as a code owner June 6, 2024 23:23
@@ -38,7 +38,7 @@ export type LocaleRequiredWrapper = (props: {
*/
export function withLocaleRequired<Props>(
/** Path containing locale files */
localePathPart?: LocalePathPartOrThunk,
localePathPart?: LocalePathPartOrThunk | LocalePathPartOrThunk[],
Copy link
Contributor

@jpage-godaddy jpage-godaddy Jun 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're interested, there's a MaybeMultiple (or perhaps it was called MaybeArray) type that you can import and use.

@jpage-godaddy
Copy link
Contributor

You'll also need to update the server-side code for withLocaleRequired so it includes all of the strings in the server payload; currently it expects just a single thunk or string. You've only covered the client-side loading.

@eliebman-godaddy
Copy link
Contributor Author

You'll also need to update the server-side code for withLocaleRequired so it includes all of the strings in the server payload; currently it expects just a single thunk or string. You've only covered the client-side loading.

@jpage-godaddy, It seems like serverside, in getInitialProps, we just pass localePathPart directly to serverLoadData, which looks like it already supports arrays

Is there some other server-side code I'm missing?

@mmason2-godaddy mmason2-godaddy self-requested a review June 7, 2024 15:35
@mmason2-godaddy mmason2-godaddy merged commit b80acff into godaddy:main Jun 7, 2024
3 checks passed
mmason2-godaddy pushed a commit that referenced this pull request Jun 7, 2024
…equired`, and `LocaleRequired` (#779)

* Update useLocalesRequired, withLocaleRequired, and LocaleRequired to accept an array as `localesPath`

* Update Unit Tests

* Update Docs
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.

3 participants