From a6691c71c655c4f8daeb9aac4103c57b4f508822 Mon Sep 17 00:00:00 2001 From: Jarda Snajdr Date: Fri, 19 Apr 2024 11:40:43 +0200 Subject: [PATCH 1/3] ObservableMap: optimize unsubscribe and add unit tests --- .../bubbles-virtually/observable-map.ts | 41 +++++---- .../src/slot-fill/test/observable-map.js | 87 +++++++++++++++++++ 2 files changed, 111 insertions(+), 17 deletions(-) create mode 100644 packages/components/src/slot-fill/test/observable-map.js diff --git a/packages/components/src/slot-fill/bubbles-virtually/observable-map.ts b/packages/components/src/slot-fill/bubbles-virtually/observable-map.ts index d6ef70dac5830c..bb2e8b0bd9707e 100644 --- a/packages/components/src/slot-fill/bubbles-virtually/observable-map.ts +++ b/packages/components/src/slot-fill/bubbles-virtually/observable-map.ts @@ -10,6 +10,10 @@ export type ObservableMap< K, V > = { subscribe( name: K, listener: () => void ): () => void; }; +/** + * A key/value map where the individual entries are observable by subscribing to them + * with the `subscribe` methods. + */ export function observableMap< K, V >(): ObservableMap< K, V > { const map = new Map< K, V >(); const listeners = new Map< K, Set< () => void > >(); @@ -24,18 +28,14 @@ export function observableMap< K, V >(): ObservableMap< K, V > { } } - function unsubscribe( name: K, listener: () => void ) { - return () => { - const list = listeners.get( name ); - if ( ! list ) { - return; - } + function getListeners( name: K ) { + let list = listeners.get( name ); + if ( ! list ) { + list = new Set(); + listeners.set( name, list ); + } - list.delete( listener ); - if ( list.size === 0 ) { - listeners.delete( name ); - } - }; + return list; } return { @@ -51,18 +51,25 @@ export function observableMap< K, V >(): ObservableMap< K, V > { callListeners( name ); }, subscribe( name, listener ) { - let list = listeners.get( name ); - if ( ! list ) { - list = new Set(); - listeners.set( name, list ); - } + const list = getListeners( name ); list.add( listener ); - return unsubscribe( name, listener ); + return () => { + list.delete( listener ); + if ( list.size === 0 ) { + listeners.delete( name ); + } + }; }, }; } +/** + * React hook that lets you observe an individual entry in an `ObservableMap`. + * + * @param map The `ObservableMap` to observe. + * @param name The map key to observe. + */ export function useObservableValue< K, V >( map: ObservableMap< K, V >, name: K diff --git a/packages/components/src/slot-fill/test/observable-map.js b/packages/components/src/slot-fill/test/observable-map.js new file mode 100644 index 00000000000000..363090551eae51 --- /dev/null +++ b/packages/components/src/slot-fill/test/observable-map.js @@ -0,0 +1,87 @@ +/** + * External dependencies + */ +import { render, screen, act } from '@testing-library/react'; + +/** + * Internal dependencies + */ +import { + observableMap, + useObservableValue, +} from '../bubbles-virtually/observable-map'; + +describe( 'ObservableMap', () => { + test( 'should observe individual values', () => { + const map = observableMap(); + let notifsFromA = 0; + let notifsFromB = 0; + + const unsubA = map.subscribe( 'a', () => { + notifsFromA++; + } ); + const unsubB = map.subscribe( 'b', () => { + notifsFromB++; + } ); + + // check that setting `a` doesn't notify the `b` listener + map.set( 'a', 1 ); + expect( notifsFromA ).toBe( 1 ); + expect( notifsFromB ).toBe( 0 ); + + // check that setting `b` doesn't notify the `a` listener + map.set( 'b', 2 ); + expect( notifsFromA ).toBe( 1 ); + expect( notifsFromB ).toBe( 1 ); + + // check that `delete` triggers notifications, too + map.delete( 'a' ); + expect( notifsFromA ).toBe( 2 ); + expect( notifsFromB ).toBe( 1 ); + + // check that the subscription survived the `delete` + map.set( 'a', 2 ); + expect( notifsFromA ).toBe( 3 ); + expect( notifsFromB ).toBe( 1 ); + + // check that unsubscription really works. + unsubA(); + unsubB(); + map.set( 'a', 3 ); + expect( notifsFromA ).toBe( 3 ); + expect( notifsFromB ).toBe( 1 ); + } ); +} ); + +describe( 'useObservableValue', () => { + test( 'reacts only to the specified key', () => { + const map = observableMap(); + map.set( 'a', 1 ); + + let renders = 0; + function MapUI() { + const value = useObservableValue( map, 'a' ); + renders++; + return
value is { value }
; + } + + render( ); + expect( screen.getByText( /^value is/ ) ).toHaveTextContent( + 'value is 1' + ); + expect( renders ).toBe( 1 ); + + act( () => { + map.set( 'a', 2 ); + } ); + expect( screen.getByText( /^value is/ ) ).toHaveTextContent( + 'value is 2' + ); + expect( renders ).toBe( 2 ); + + act( () => { + map.set( 'b', 1 ); + } ); + expect( renders ).toBe( 2 ); + } ); +} ); From 5ae7a802571efb15a093d829d2d74d6135c08bbb Mon Sep 17 00:00:00 2001 From: Jarda Snajdr Date: Fri, 19 Apr 2024 14:40:03 +0200 Subject: [PATCH 2/3] Use jest.fn to count function calls --- .../src/slot-fill/test/observable-map.js | 48 +++++++++---------- 1 file changed, 22 insertions(+), 26 deletions(-) diff --git a/packages/components/src/slot-fill/test/observable-map.js b/packages/components/src/slot-fill/test/observable-map.js index 363090551eae51..ee3b3533bdd3ca 100644 --- a/packages/components/src/slot-fill/test/observable-map.js +++ b/packages/components/src/slot-fill/test/observable-map.js @@ -14,42 +14,39 @@ import { describe( 'ObservableMap', () => { test( 'should observe individual values', () => { const map = observableMap(); - let notifsFromA = 0; - let notifsFromB = 0; - const unsubA = map.subscribe( 'a', () => { - notifsFromA++; - } ); - const unsubB = map.subscribe( 'b', () => { - notifsFromB++; - } ); + const listenerA = jest.fn(); + const listenerB = jest.fn(); + + const unsubA = map.subscribe( 'a', listenerA ); + const unsubB = map.subscribe( 'b', listenerB ); // check that setting `a` doesn't notify the `b` listener map.set( 'a', 1 ); - expect( notifsFromA ).toBe( 1 ); - expect( notifsFromB ).toBe( 0 ); + expect( listenerA ).toHaveBeenCalledTimes( 1 ); + expect( listenerB ).toHaveBeenCalledTimes( 0 ); // check that setting `b` doesn't notify the `a` listener map.set( 'b', 2 ); - expect( notifsFromA ).toBe( 1 ); - expect( notifsFromB ).toBe( 1 ); + expect( listenerA ).toHaveBeenCalledTimes( 1 ); + expect( listenerB ).toHaveBeenCalledTimes( 1 ); // check that `delete` triggers notifications, too map.delete( 'a' ); - expect( notifsFromA ).toBe( 2 ); - expect( notifsFromB ).toBe( 1 ); + expect( listenerA ).toHaveBeenCalledTimes( 2 ); + expect( listenerB ).toHaveBeenCalledTimes( 1 ); // check that the subscription survived the `delete` map.set( 'a', 2 ); - expect( notifsFromA ).toBe( 3 ); - expect( notifsFromB ).toBe( 1 ); + expect( listenerA ).toHaveBeenCalledTimes( 3 ); + expect( listenerB ).toHaveBeenCalledTimes( 1 ); - // check that unsubscription really works. + // check that unsubscription really works unsubA(); unsubB(); map.set( 'a', 3 ); - expect( notifsFromA ).toBe( 3 ); - expect( notifsFromB ).toBe( 1 ); + expect( listenerA ).toHaveBeenCalledTimes( 3 ); + expect( listenerB ).toHaveBeenCalledTimes( 1 ); } ); } ); @@ -58,18 +55,16 @@ describe( 'useObservableValue', () => { const map = observableMap(); map.set( 'a', 1 ); - let renders = 0; - function MapUI() { + const MapUI = jest.fn( () => { const value = useObservableValue( map, 'a' ); - renders++; return
value is { value }
; - } + } ); render( ); expect( screen.getByText( /^value is/ ) ).toHaveTextContent( 'value is 1' ); - expect( renders ).toBe( 1 ); + expect( MapUI ).toHaveBeenCalledTimes( 1 ); act( () => { map.set( 'a', 2 ); @@ -77,11 +72,12 @@ describe( 'useObservableValue', () => { expect( screen.getByText( /^value is/ ) ).toHaveTextContent( 'value is 2' ); - expect( renders ).toBe( 2 ); + expect( MapUI ).toHaveBeenCalledTimes( 2 ); + // check that setting unobserved map key doesn't trigger a render at all act( () => { map.set( 'b', 1 ); } ); - expect( renders ).toBe( 2 ); + expect( MapUI ).toHaveBeenCalledTimes( 2 ); } ); } ); From bf039dca32fcdbc62f2fb618302f2dc23d5c274f Mon Sep 17 00:00:00 2001 From: Jarda Snajdr Date: Fri, 19 Apr 2024 14:45:46 +0200 Subject: [PATCH 3/3] Inline the getListeners function after TS upgrade to 5.4.5 --- .../bubbles-virtually/observable-map.ts | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/packages/components/src/slot-fill/bubbles-virtually/observable-map.ts b/packages/components/src/slot-fill/bubbles-virtually/observable-map.ts index bb2e8b0bd9707e..fcf6871ceabcc8 100644 --- a/packages/components/src/slot-fill/bubbles-virtually/observable-map.ts +++ b/packages/components/src/slot-fill/bubbles-virtually/observable-map.ts @@ -28,16 +28,6 @@ export function observableMap< K, V >(): ObservableMap< K, V > { } } - function getListeners( name: K ) { - let list = listeners.get( name ); - if ( ! list ) { - list = new Set(); - listeners.set( name, list ); - } - - return list; - } - return { get( name ) { return map.get( name ); @@ -51,7 +41,11 @@ export function observableMap< K, V >(): ObservableMap< K, V > { callListeners( name ); }, subscribe( name, listener ) { - const list = getListeners( name ); + let list = listeners.get( name ); + if ( ! list ) { + list = new Set(); + listeners.set( name, list ); + } list.add( listener ); return () => {