From c824ab53b3ada7bdb373a2ecfa738a774d4c6b8a Mon Sep 17 00:00:00 2001 From: Brian Ryu Date: Wed, 3 Jun 2020 10:31:52 -0400 Subject: [PATCH 1/2] Do a smarter merge of i18n details --- .../CHANGELOG.md | 6 +- .../react-i18n-universal-provider/README.md | 13 ++- .../src/I18nUniversalProvider.tsx | 11 ++- .../src/test/I18nUniversalProvider.test.tsx | 47 +++++++++- .../src/test/utilities.test.ts | 91 +++++++++++++++++++ .../src/utilities.ts | 13 +++ 6 files changed, 170 insertions(+), 11 deletions(-) create mode 100644 packages/react-i18n-universal-provider/src/test/utilities.test.ts create mode 100644 packages/react-i18n-universal-provider/src/utilities.ts diff --git a/packages/react-i18n-universal-provider/CHANGELOG.md b/packages/react-i18n-universal-provider/CHANGELOG.md index 6f7ad21edd..b3d1edce7a 100644 --- a/packages/react-i18n-universal-provider/CHANGELOG.md +++ b/packages/react-i18n-universal-provider/CHANGELOG.md @@ -5,7 +5,11 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). - +## [Unreleased] + +### Fixed + +- Fixed merge of serialized i18n data and override values ## [1.0.28] - 2019-11-29 diff --git a/packages/react-i18n-universal-provider/README.md b/packages/react-i18n-universal-provider/README.md index 6b80e9abb5..670d37dbfd 100644 --- a/packages/react-i18n-universal-provider/README.md +++ b/packages/react-i18n-universal-provider/README.md @@ -39,13 +39,18 @@ I18nDetails { import {I18nUniversalProvider} from '@shopify/react-i18n-universal-provider'; function App({locale}: {locale?: string}) { - return {/* rest of the app */}; + return ( + + {/* rest of the app */} + + ); } ``` ### Possible Issues -#### Missing i18n manager error +#### Missing i18n manager error + ``` Error: Missing i18n manager. Make sure to use an somewhere in your React tree from the @shopify/react-i18n hook. ``` @@ -66,7 +71,7 @@ npx yarn-deduplicate --packages @shopify/react-effect yarn.lock ``` ```bash -$ yarn why @shopify/react-i18n # ensure no duplicate / unmet dependencies -yarn list # ensure no duplicate / unmet dependencies +$ yarn why @shopify/react-i18n # ensure no duplicate / unmet dependencies +yarn list # ensure no duplicate / unmet dependencies yarn install ``` diff --git a/packages/react-i18n-universal-provider/src/I18nUniversalProvider.tsx b/packages/react-i18n-universal-provider/src/I18nUniversalProvider.tsx index 7954c56ed8..ebdea129ae 100644 --- a/packages/react-i18n-universal-provider/src/I18nUniversalProvider.tsx +++ b/packages/react-i18n-universal-provider/src/I18nUniversalProvider.tsx @@ -3,6 +3,8 @@ import {useLazyRef} from '@shopify/react-hooks'; import {useSerialized, useHtmlAttributes} from '@shopify/react-html'; import {I18nContext, I18nDetails, I18nManager} from '@shopify/react-i18n'; +import {combinedI18nDetails} from './utilities'; + interface Props extends Partial { children?: React.ReactNode; } @@ -24,11 +26,10 @@ export function I18nUniversalProvider({ }: Props) { const [serialized, Serialize] = useSerialized('i18n'); - const i18nDetails: I18nDetails = { - locale: explicitI18nDetails.fallbackLocale || 'en', - ...(serialized ? serialized : {}), - ...explicitI18nDetails, - }; + const i18nDetails: I18nDetails = combinedI18nDetails( + serialized, + explicitI18nDetails, + ); const manager = useLazyRef( () => diff --git a/packages/react-i18n-universal-provider/src/test/I18nUniversalProvider.test.tsx b/packages/react-i18n-universal-provider/src/test/I18nUniversalProvider.test.tsx index caecd01d3e..a8fc286b00 100644 --- a/packages/react-i18n-universal-provider/src/test/I18nUniversalProvider.test.tsx +++ b/packages/react-i18n-universal-provider/src/test/I18nUniversalProvider.test.tsx @@ -30,7 +30,6 @@ describe('', () => { ); expect(i18n).toProvideReactContext(I18nContext, expect.any(I18nManager)); - expect(i18n).toProvideReactContext( I18nContext, expect.objectContaining({ @@ -81,4 +80,50 @@ describe('', () => { }), ); }); + + it('overrides serialized data with defined overrides', async () => { + const htmlManager = new HtmlManager(); + const locale = faker.random.locale(); + const currency = faker.finance.currencyCode(); + const country = faker.address.country(); + const timezone = faker.lorem.word(); + + await extract( + , + { + decorate: (element: React.ReactNode) => ( + + {element} + + ), + }, + ); + + const overrideDetails = { + locale: faker.random.locale(), + currency: undefined, + country: faker.address.country(), + timezone: undefined, + }; + const i18n = mount(, { + htmlManager, + }); + + expect(i18n).toProvideReactContext( + I18nContext, + expect.objectContaining({ + details: expect.objectContaining({ + locale: overrideDetails.locale, + timezone, + currency, + country: overrideDetails.country, + }), + }), + ); + }); }); diff --git a/packages/react-i18n-universal-provider/src/test/utilities.test.ts b/packages/react-i18n-universal-provider/src/test/utilities.test.ts new file mode 100644 index 0000000000..5ece5a8f5c --- /dev/null +++ b/packages/react-i18n-universal-provider/src/test/utilities.test.ts @@ -0,0 +1,91 @@ +import faker from 'faker'; + +import {combinedI18nDetails} from '../utilities'; + +describe('combinedI18nDetails()', () => { + it('merges two details objects together', () => { + const details = { + locale: faker.random.locale(), + country: faker.address.country(), + }; + const overrides = { + currency: faker.finance.currencyCode(), + }; + + const combinedDetails = combinedI18nDetails(details, overrides); + + expect(combinedDetails).toHaveProperty('locale'); + expect(combinedDetails).toHaveProperty('country'); + expect(combinedDetails).toHaveProperty('currency'); + }); + + it('overrides fields with defined overrides', () => { + const locale = faker.random.locale(); + const country = faker.address.country(); + const currency = faker.finance.currencyCode(); + const details = { + locale, + country, + currency, + }; + const overrideCurrency = faker.finance.currencyCode(); + const overrides = { + locale: undefined, + country: undefined, + currency: overrideCurrency, + }; + + const combinedDetails = combinedI18nDetails(details, overrides); + + expect(combinedDetails).toHaveProperty('locale', locale); + expect(combinedDetails).toHaveProperty('country', country); + expect(combinedDetails).toHaveProperty('currency', overrideCurrency); + }); + + describe('locale', () => { + it('returns a details object with a locale field', () => { + const locale = faker.random.locale(); + const fallbackLocale = faker.random.locale(); + const details = {locale}; + const overrides = {locale: undefined, fallbackLocale}; + + expect(combinedI18nDetails(details, overrides)).toHaveProperty( + 'locale', + locale, + ); + }); + + it('favours an override locale if one is specified', () => { + const locale = faker.random.locale(); + const overrideLocale = faker.random.locale(); + const details = {locale}; + const overrides = {locale: overrideLocale}; + + expect(combinedI18nDetails(details, overrides)).toHaveProperty( + 'locale', + overrideLocale, + ); + }); + + it('returns a details object with a fallback locale field if one is specified', () => { + const fallbackLocale = faker.random.locale(); + const details = {}; + const overrides = {fallbackLocale}; + + expect(combinedI18nDetails(details, overrides)).toHaveProperty( + 'locale', + fallbackLocale, + ); + }); + + it('returns a details object with a default locale field if none is specified', () => { + const details = {}; + const overrides = {}; + + expect(combinedI18nDetails(details, overrides)).toHaveProperty( + 'locale', + 'en', + ); + }); + }); +}); diff --git a/packages/react-i18n-universal-provider/src/utilities.ts b/packages/react-i18n-universal-provider/src/utilities.ts new file mode 100644 index 0000000000..c173217198 --- /dev/null +++ b/packages/react-i18n-universal-provider/src/utilities.ts @@ -0,0 +1,13 @@ +function pruneUndefinedFields(obj = {}) { + Object.keys(obj).forEach(key => obj[key] === undefined && delete obj[key]); + + return obj; +} + +export function combinedI18nDetails(details, overrides) { + return { + locale: overrides.fallbackLocale || 'en', + ...pruneUndefinedFields(details), + ...pruneUndefinedFields(overrides), + }; +} From d7ef78e51288c36e522b06a407fe37ad7a9ef3d2 Mon Sep 17 00:00:00 2001 From: Brian Ryu Date: Wed, 3 Jun 2020 15:33:48 -0400 Subject: [PATCH 2/2] Return new object --- packages/react-i18n-universal-provider/src/utilities.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/react-i18n-universal-provider/src/utilities.ts b/packages/react-i18n-universal-provider/src/utilities.ts index c173217198..97ce77fe85 100644 --- a/packages/react-i18n-universal-provider/src/utilities.ts +++ b/packages/react-i18n-universal-provider/src/utilities.ts @@ -1,7 +1,11 @@ function pruneUndefinedFields(obj = {}) { - Object.keys(obj).forEach(key => obj[key] === undefined && delete obj[key]); + const objCopy = {...obj}; - return obj; + Object.keys(objCopy).forEach( + key => objCopy[key] === undefined && delete objCopy[key], + ); + + return objCopy; } export function combinedI18nDetails(details, overrides) {