From 41b17d5cfda1289260d0bb47cbdb152b881e672b Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 9 May 2025 14:05:35 -0400 Subject: [PATCH 1/5] Convert i18n events handling to subscriber model --- .../calypso-i18n-provider/index.tsx | 6 +- .../components/community-translator/index.jsx | 4 +- .../layout/community-translator/launcher.jsx | 4 +- client/lib/i18n-utils/empathy-mode.js | 2 +- client/lib/translator-jumpstart/index.js | 2 +- packages/i18n-calypso/package.json | 1 - packages/i18n-calypso/src/i18n.js | 28 +++----- packages/i18n-calypso/src/index.ts | 4 -- packages/i18n-calypso/src/localize.js | 3 +- packages/i18n-calypso/src/rtl.js | 5 +- packages/i18n-calypso/src/test/index.js | 68 +++++++++++++++++++ packages/i18n-calypso/src/types/index.d.ts | 5 +- packages/i18n-calypso/src/use-translate.js | 3 +- yarn.lock | 1 - 14 files changed, 92 insertions(+), 44 deletions(-) diff --git a/client/components/calypso-i18n-provider/index.tsx b/client/components/calypso-i18n-provider/index.tsx index 560c989acf01..13905c5964c4 100644 --- a/client/components/calypso-i18n-provider/index.tsx +++ b/client/components/calypso-i18n-provider/index.tsx @@ -19,11 +19,7 @@ const CalypsoI18nProvider: FunctionComponent< { i18n?: I18N; children?: React.Re setLocaleSlug( i18n.getLocaleSlug() ); }; - i18n.on( 'change', onChange ); - - return () => { - i18n.off( 'change', onChange ); - }; + return i18n.subscribe( onChange ); }, [ i18n ] ); useEffect( () => { diff --git a/client/components/community-translator/index.jsx b/client/components/community-translator/index.jsx index 9e2f9c7eb2d1..7a7515afec0f 100644 --- a/client/components/community-translator/index.jsx +++ b/client/components/community-translator/index.jsx @@ -31,11 +31,11 @@ class CommunityTranslator extends Component { // callback when translated component changes. // the callback is overwritten by the translator on load/unload, so we're returning it within an anonymous function. i18n.registerComponentUpdateHook( () => {} ); - i18n.on( 'change', this.refresh ); + this.i18nUnsubscribe = i18n.subscribe( this.refresh ); } componentWillUnmount() { - i18n.off( 'change', this.refresh ); + this.i18nUnsubscribe(); } setLanguage() { diff --git a/client/layout/community-translator/launcher.jsx b/client/layout/community-translator/launcher.jsx index cec8cebafd49..d23976abea38 100644 --- a/client/layout/community-translator/launcher.jsx +++ b/client/layout/community-translator/launcher.jsx @@ -59,12 +59,12 @@ class TranslatorLauncher extends Component { highlightRef = createRef(); componentDidMount() { - i18n.on( 'change', this.onI18nChange ); + this.i18nUnsubscribe = i18n.subscribe( this.onI18nChange ); window.addEventListener( 'keydown', this.handleKeyDown ); } componentWillUnmount() { - i18n.off( 'change', this.onI18nChange ); + this.i18nUnsubscribe(); window.removeEventListener( 'keydown', this.handleKeyDown ); } diff --git a/client/lib/i18n-utils/empathy-mode.js b/client/lib/i18n-utils/empathy-mode.js index 5dafb7dc0864..ceaaf3aeab28 100644 --- a/client/lib/i18n-utils/empathy-mode.js +++ b/client/lib/i18n-utils/empathy-mode.js @@ -3,7 +3,7 @@ import i18n, { I18N, translate } from 'i18n-calypso'; let defaultUntranslatedPlacehoder = translate( "I don't understand" ); // keep `defaultUntranslatedPlacehoder` in sync with i18n changes -i18n.on( 'change', () => { +i18n.subscribe( () => { defaultUntranslatedPlacehoder = translate( "I don't understand" ); } ); diff --git a/client/lib/translator-jumpstart/index.js b/client/lib/translator-jumpstart/index.js index 3060be4bbbc1..99c12a7afdee 100644 --- a/client/lib/translator-jumpstart/index.js +++ b/client/lib/translator-jumpstart/index.js @@ -328,6 +328,6 @@ export function trackTranslatorStatus( isTranslatorEnabled ) { } // re-initialize when new locale data is loaded -i18n.on( 'change', communityTranslatorJumpstart.init.bind( communityTranslatorJumpstart ) ); +i18n.subscribe( communityTranslatorJumpstart.init.bind( communityTranslatorJumpstart ) ); export default communityTranslatorJumpstart; diff --git a/packages/i18n-calypso/package.json b/packages/i18n-calypso/package.json index ea300fe441f7..5073546c9a9e 100644 --- a/packages/i18n-calypso/package.json +++ b/packages/i18n-calypso/package.json @@ -29,7 +29,6 @@ "@tannin/sprintf": "^1.1.0", "@wordpress/compose": "^7.23.0", "debug": "^4.4.0", - "events": "^3.3.0", "hash.js": "^1.1.7", "lru": "^3.1.0", "tannin": "^1.2.0", diff --git a/packages/i18n-calypso/src/i18n.js b/packages/i18n-calypso/src/i18n.js index 6e2672213cd9..5d9695b72761 100644 --- a/packages/i18n-calypso/src/i18n.js +++ b/packages/i18n-calypso/src/i18n.js @@ -1,4 +1,3 @@ -import { EventEmitter } from 'events'; import interpolateComponents from '@automattic/interpolate-components'; import sprintf from '@tannin/sprintf'; import debugFactory from 'debug'; @@ -145,11 +144,7 @@ function I18N() { }; this.componentUpdateHooks = []; this.translateHooks = []; - this.stateObserver = new EventEmitter(); - // Because the higher-order component can wrap a ton of React components, - // we need to bump the number of listeners to infinity and beyond - // FIXME: still valid? - this.stateObserver.setMaxListeners( 0 ); + this.subscribers = []; // default configuration this.configure(); } @@ -191,16 +186,15 @@ I18N.prototype.geolocateCurrencySymbol = async function ( callback ) { callback?.( 'string' === typeof geoData?.country_short ? geoData.country_short : '' ); }; -I18N.prototype.on = function ( ...args ) { - this.stateObserver.on( ...args ); -}; - -I18N.prototype.off = function ( ...args ) { - this.stateObserver.off( ...args ); +I18N.prototype.subscribe = function ( callback ) { + this.subscribers.push( callback ); + return () => { + this.subscribers = this.subscribers.filter( ( c ) => c !== callback ); + }; }; -I18N.prototype.emit = function ( ...args ) { - this.stateObserver.emit( ...args ); +I18N.prototype.emitChange = function () { + this.subscribers.forEach( ( callback ) => callback() ); }; /** @@ -304,7 +298,7 @@ I18N.prototype.setLocale = function ( localeData ) { this.state.tannin = new Tannin( { [ domain_key ]: this.state.locale } ); - this.stateObserver.emit( 'change' ); + this.emitChange(); }; I18N.prototype.getLocale = function () { @@ -347,7 +341,7 @@ I18N.prototype.addTranslations = function ( localeData ) { } } - this.stateObserver.emit( 'change' ); + this.emitChange(); }; /** @@ -452,7 +446,7 @@ I18N.prototype.translate = function () { */ I18N.prototype.reRenderTranslations = function () { debug( 'Re-rendering all translations due to external request' ); - this.stateObserver.emit( 'change' ); + this.emitChange(); }; I18N.prototype.registerComponentUpdateHook = function ( callback ) { diff --git a/packages/i18n-calypso/src/index.ts b/packages/i18n-calypso/src/index.ts index 80c156575225..23018840c5ed 100644 --- a/packages/i18n-calypso/src/index.ts +++ b/packages/i18n-calypso/src/index.ts @@ -24,10 +24,6 @@ export const reRenderTranslations = i18n.reRenderTranslations.bind( i18n ); export const registerComponentUpdateHook = i18n.registerComponentUpdateHook.bind( i18n ); export const registerTranslateHook = i18n.registerTranslateHook.bind( i18n ); export const state = i18n.state; -export const stateObserver = i18n.stateObserver; -export const on = i18n.on.bind( i18n ); -export const off = i18n.off.bind( i18n ); -export const emit = i18n.emit.bind( i18n ); export const fixMe = i18n.fixMe.bind( i18n ); export type * from './types'; diff --git a/packages/i18n-calypso/src/localize.js b/packages/i18n-calypso/src/localize.js index b821d84bf6ae..7787ea5d9dba 100644 --- a/packages/i18n-calypso/src/localize.js +++ b/packages/i18n-calypso/src/localize.js @@ -19,8 +19,7 @@ export default function localize( ComposedComponent ) { const [ counter, setCounter ] = useState( 0 ); useEffect( () => { const onChange = () => setCounter( ( c ) => c + 1 ); - i18n.on( 'change', onChange ); - return () => i18n.off( 'change', onChange ); + return i18n.subscribe( onChange ); }, [ i18n ] ); const i18nProps = useMemo( () => bindI18nProps( i18n, counter ), [ i18n, counter ] ); diff --git a/packages/i18n-calypso/src/rtl.js b/packages/i18n-calypso/src/rtl.js index 3d80daf1e477..05b04852a3a9 100644 --- a/packages/i18n-calypso/src/rtl.js +++ b/packages/i18n-calypso/src/rtl.js @@ -12,8 +12,7 @@ export function useRtl() { return i18n.isRtl(); }, subscribe( callback ) { - i18n.on( 'change', callback ); - return () => i18n.off( 'change', callback ); + return i18n.subscribe( callback ); }, } ), [ i18n ] @@ -24,7 +23,7 @@ export function useRtl() { export const withRtl = createHigherOrderComponent( ( WrappedComponent ) => - forwardRef( ( props, ref ) => { + forwardRef( function WrappedRtlComponent( props, ref ) { const isRtl = useRtl(); return ; } ), diff --git a/packages/i18n-calypso/src/test/index.js b/packages/i18n-calypso/src/test/index.js index ab0cbd1e54ab..cb2e9d85b35d 100644 --- a/packages/i18n-calypso/src/test/index.js +++ b/packages/i18n-calypso/src/test/index.js @@ -24,6 +24,15 @@ describe( 'I18n', function () { } ); describe( 'setLocale()', function () { + it( 'should emit a change event', () => { + const callback = jest.fn(); + i18n.subscribe( callback ); + + i18n.setLocale(); + + expect( callback ).toHaveBeenCalled(); + } ); + describe( 'adding a new locale source from the same language', function () { beforeEach( function () { i18n.setLocale( { @@ -191,6 +200,7 @@ describe( 'I18n', function () { expect( translate( 'test-does-not-exist' ) ).toBe( 'translation3' ); } ); + it( 'should return the new translation if it has been overwritten', function () { i18n.addTranslations( { 'test-will-overwrite': [ 'not-translation1' ], @@ -198,6 +208,15 @@ describe( 'I18n', function () { expect( translate( 'test-will-overwrite' ) ).toBe( 'not-translation1' ); } ); + + it( 'should emit a change event', () => { + const callback = jest.fn(); + i18n.subscribe( callback ); + + i18n.addTranslations( {} ); + + expect( callback ).toHaveBeenCalled(); + } ); } ); } ); @@ -266,6 +285,55 @@ describe( 'I18n', function () { } ); } ); + describe( 'emitChange()', () => { + it( 'should call all subscribed callbacks', () => { + const callback1 = jest.fn(); + const callback2 = jest.fn(); + + i18n.subscribe( callback1 ); + i18n.subscribe( callback2 ); + + i18n.emitChange(); + + expect( callback1 ).toHaveBeenCalled(); + expect( callback2 ).toHaveBeenCalled(); + } ); + } ); + + describe( 'reRenderTranslations()', () => { + it( 'should call subscriber callback', () => { + const callback = jest.fn(); + + i18n.subscribe( callback ); + + i18n.reRenderTranslations(); + + expect( callback ).toHaveBeenCalled(); + } ); + } ); + + describe( 'subscribe()', () => { + it( 'should call the callback whenever a change is emitted', () => { + const callback = jest.fn(); + + i18n.subscribe( callback ); + i18n.setLocale(); + + expect( callback ).toHaveBeenCalled(); + } ); + + it( 'should return an unsubscribe function', () => { + const callback = jest.fn(); + + const unsubscribe = i18n.subscribe( callback ); + unsubscribe(); + + i18n.setLocale(); + + expect( callback ).not.toHaveBeenCalled(); + } ); + } ); + describe( 'hashed locale data', function () { it( 'should find keys when looked up by simple hash', function () { i18n.setLocale( { diff --git a/packages/i18n-calypso/src/types/index.d.ts b/packages/i18n-calypso/src/types/index.d.ts index c75c16cd4a09..523cdc4a98dd 100644 --- a/packages/i18n-calypso/src/types/index.d.ts +++ b/packages/i18n-calypso/src/types/index.d.ts @@ -132,9 +132,8 @@ export interface I18N { registerTranslateHook( hook: TranslateHook ): void; registerComponentUpdateHook( hook: ComponentUpdateHook ): void; - on( eventName: string, listener: EventListener ): void; - off( eventName: string, listener: EventListener ): void; - emit( eventName: string, ...payload: any ): void; + subscribe( callback: () => any ): () => void; + emitChange(): void; /** * Returns `newCopy` if given `text` is translated or locale is English, otherwise returns the `oldCopy`. diff --git a/packages/i18n-calypso/src/use-translate.js b/packages/i18n-calypso/src/use-translate.js index 38f1160af490..b7319a9d79d2 100644 --- a/packages/i18n-calypso/src/use-translate.js +++ b/packages/i18n-calypso/src/use-translate.js @@ -13,8 +13,7 @@ export default function useTranslate() { useEffect( () => { const onChange = () => setCounter( ( c ) => c + 1 ); - i18n.on( 'change', onChange ); - return () => i18n.off( 'change', onChange ); + return i18n.subscribe( onChange ); }, [ i18n ] ); return useMemo( () => bindTranslate( i18n, counter ), [ i18n, counter ] ); diff --git a/yarn.lock b/yarn.lock index 41d72f439749..b62f380b1b15 100644 --- a/yarn.lock +++ b/yarn.lock @@ -21885,7 +21885,6 @@ __metadata: "@types/react": "npm:^18.3.20" "@wordpress/compose": "npm:^7.23.0" debug: "npm:^4.4.0" - events: "npm:^3.3.0" hash.js: "npm:^1.1.7" lru: "npm:^3.1.0" react: "npm:^18.3.1" From 7cd2a0880acaa2def9da674ac84b1976d9e7bab5 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 9 May 2025 15:08:18 -0400 Subject: [PATCH 2/5] Add additional test coverage --- packages/i18n-calypso/src/test/index.js | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/packages/i18n-calypso/src/test/index.js b/packages/i18n-calypso/src/test/index.js index cb2e9d85b35d..a1e644c252a4 100644 --- a/packages/i18n-calypso/src/test/index.js +++ b/packages/i18n-calypso/src/test/index.js @@ -298,6 +298,23 @@ describe( 'I18n', function () { expect( callback1 ).toHaveBeenCalled(); expect( callback2 ).toHaveBeenCalled(); } ); + + it( 'should not call unsubscribed callbacks', () => { + const callback1 = jest.fn(); + const callback2 = jest.fn(); + const callback3 = jest.fn(); + + i18n.subscribe( callback1 ); + const unsubscribe2 = i18n.subscribe( callback2 ); + i18n.subscribe( callback3 ); + + unsubscribe2(); + i18n.emitChange(); + + expect( callback1 ).toHaveBeenCalled(); + expect( callback2 ).not.toHaveBeenCalled(); + expect( callback3 ).toHaveBeenCalled(); + } ); } ); describe( 'reRenderTranslations()', () => { From ee3e0bb5dfdd3802d09b802caa92eef0999fcf43 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 9 May 2025 15:08:56 -0400 Subject: [PATCH 3/5] Microoptimize subscriber removal --- packages/i18n-calypso/src/i18n.js | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/packages/i18n-calypso/src/i18n.js b/packages/i18n-calypso/src/i18n.js index 5d9695b72761..bbe1ea6a2144 100644 --- a/packages/i18n-calypso/src/i18n.js +++ b/packages/i18n-calypso/src/i18n.js @@ -26,6 +26,24 @@ const translationLookup = [ const hashCache = {}; +/** + * Efficiently removes an item from an array, with the assumption that the item appears only once. + * @param {Array} array The array to remove the item from. + * @param {I} item The item to remove from the array. + * @template I + */ +function removeOne( array, item ) { + for ( let i = 0; i < array.length; i++ ) { + if ( array[ i ] === item ) { + for ( ; i < array.length - 1; i++ ) { + array[ i ] = array[ i + 1 ]; + } + array.pop(); + return; + } + } +} + // raise a console warning function warn() { if ( ! I18N.throwErrors ) { @@ -188,9 +206,7 @@ I18N.prototype.geolocateCurrencySymbol = async function ( callback ) { I18N.prototype.subscribe = function ( callback ) { this.subscribers.push( callback ); - return () => { - this.subscribers = this.subscribers.filter( ( c ) => c !== callback ); - }; + return () => removeOne( this.subscribers, callback ); }; I18N.prototype.emitChange = function () { From b0355f11ce9553782230a84395780005b61df954 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 9 May 2025 15:09:08 -0400 Subject: [PATCH 4/5] Update changelog for i18n-calypso --- packages/i18n-calypso/CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/i18n-calypso/CHANGELOG.md b/packages/i18n-calypso/CHANGELOG.md index 49eb7d3aa914..3eb667b17b2b 100644 --- a/packages/i18n-calypso/CHANGELOG.md +++ b/packages/i18n-calypso/CHANGELOG.md @@ -1,3 +1,9 @@ +## Trunk + +- Breaking: Emitter object replaced with subscriber + - Before: `i18n.on( 'change', callback ); i18n.off( 'change', callback );` + - After: `const unsubscribe = i18n.subscribe( callback ); unsubscribe();` + ## 7.5.0 - Add `translationOptions` to fixMe so it can work on translations with context. From 9f26ca1e6957cf3fb3b6639a2603114ac37eff3a Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 9 May 2025 15:30:00 -0400 Subject: [PATCH 5/5] Remove types for eliminated methods --- packages/i18n-calypso/src/types/index.d.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/i18n-calypso/src/types/index.d.ts b/packages/i18n-calypso/src/types/index.d.ts index 523cdc4a98dd..9e01515225e3 100644 --- a/packages/i18n-calypso/src/types/index.d.ts +++ b/packages/i18n-calypso/src/types/index.d.ts @@ -179,9 +179,6 @@ export declare const isRtl: typeof i18n.isRtl; export declare const defaultLocaleSlug: typeof i18n.defaultLocaleSlug; export declare const registerTranslateHook: typeof i18n.registerTranslateHook; export declare const registerComponentUpdateHook: typeof i18n.registerComponentUpdateHook; -export declare const on: typeof i18n.on; -export declare const off: typeof i18n.off; -export declare const emit: typeof i18n.emit; export declare const fixMe: typeof i18n.fixMe; export interface LocalizeProps {