improve observables usage (#206)

- use custom isEqualJSON instead of JSON.stringify (for which key order
matters)
- use switchMap instead of mergeMap to stop unwanted emissions

client/browser/src/contentScript/contentScript.main.ts

@@ -6,7 +6,7 @@ import type { AnnotationsParams } from '@openctx/provider'
 import deepEqual from 'deep-equal'
 import { background } from '../browser-extension/web-extension-api/runtime.js'
 import './contentScript.main.css'
-import { combineLatest, distinctUntilChanged, mergeMap, throttleTime } from '@openctx/client/observable'
+import { combineLatest, distinctUntilChanged, switchMap, throttleTime } from '@openctx/client/observable'
 import type { Observable } from 'observable-fns'
 import { debugTap } from './debug.js'
 import { injectOnGitHubCodeView } from './github/codeView.js'
@@ -23,7 +23,7 @@ const INJECTORS: Injector[] = [injectOnGitHubCodeView, injectOnGitHubPullRequest
 
 const subscription = locationChanges
     .pipe(
-        mergeMap(location =>
+        switchMap(location =>
             combineLatest(INJECTORS.map(injector => injector(location, annotationsChanges))),
         ),
     )

client/browser/src/contentScript/github/codeView.ts

@@ -1,5 +1,12 @@
 import type { Annotation, AnnotationsParams } from '@openctx/client'
-import { EMPTY, combineLatest, debounceTime, mergeMap, startWith, tap } from '@openctx/client/observable'
+import {
+    EMPTY,
+    combineLatest,
+    debounceTime,
+    startWith,
+    switchMap,
+    tap,
+} from '@openctx/client/observable'
 import { createChipList } from '@openctx/ui-standalone'
 import { Observable, map } from 'observable-fns'
 import { toLineRangeStrings } from '../../shared/util/toLineRangeStrings.js'
@@ -33,7 +40,7 @@ export function injectOnGitHubCodeView(
         withDOMElement<HTMLTextAreaElement>('#read-only-cursor-text-area'),
         withDOMElement<HTMLElement>('react-app[app-name="react-code-view"]'),
     ]).pipe(
-        mergeMap(([cursorTextArea, reactCodeView]) => {
+        switchMap(([cursorTextArea, reactCodeView]) => {
             interface GitHubCodeView {
                 cursorTextArea: HTMLTextAreaElement
                 reactCodeView: HTMLElement

client/browser/src/contentScript/github/pullRequestFilesView.ts

@@ -1,5 +1,5 @@
 import type { Annotation, AnnotationsParams } from '@openctx/client'
-import { EMPTY, combineLatest, fromEvent, mergeMap, startWith, tap } from '@openctx/client/observable'
+import { EMPTY, combineLatest, fromEvent, startWith, switchMap, tap } from '@openctx/client/observable'
 import { createChipList } from '@openctx/ui-standalone'
 import { type Observable, filter, map } from 'observable-fns'
 import { DEBUG, debugTap } from '../debug.js'
@@ -30,7 +30,7 @@ export function injectOnGitHubPullRequestFilesView(
         withDOMElements<HTMLElement>('.diff-view .file'),
         clicksThatInvalidateDiffViewData.pipe(startWith(undefined)),
     ]).pipe(
-        mergeMap(([fileEls]) => {
+        switchMap(([fileEls]) => {
             const diffData = getDiffViewData(fileEls)
             if (DEBUG) {
                 console.log('diffData', diffData)
@@ -110,7 +110,7 @@ const clicksThatInvalidateDiffViewData: Observable<void> = fromEvent(document.bo
 
     // Wait for it to show up. Mark .blob-expanded elements that we've seen so that this works for
     // multiple expansions.
-    mergeMap(() =>
+    switchMap(() =>
         withDOMElements('tr.blob-expanded:not(.octx-seen)').pipe(
             tap(els => {
                 for (const el of els) {

client/vscode-lib/src/controller.ts

@@ -11,8 +11,8 @@ import {
 import {
     combineLatest,
     isObservableOrInteropObservable,
-    mergeMap,
     promiseToObservable,
+    switchMap,
 } from '@openctx/client/observable'
 import { Observable, type ObservableLike, map } from 'observable-fns'
 import * as vscode from 'vscode'
@@ -107,12 +107,12 @@ export function createController({
                 ? vscode.Uri.parse(resource)
                 : vscode.workspace.workspaceFolders?.[0]?.uri
             return observeWorkspaceConfigurationChanges('openctx', scope).pipe(
-                mergeMap(() => promiseToObservable(getConfiguration(scope))),
+                switchMap(() => promiseToObservable(getConfiguration(scope))),
             )
         },
         authInfo: getAuthInfo
             ? provider =>
-                  secrets.pipe(mergeMap(secrets => promiseToObservable(getAuthInfo(secrets, provider))))
+                  secrets.pipe(switchMap(secrets => promiseToObservable(getAuthInfo(secrets, provider))))
             : undefined,
         makeRange,
         logger: message => outputChannel.appendLine(message),

lib/client/src/api.test.ts

@@ -351,7 +351,7 @@ describe('observeAnnotations', () => {
     test('config changes', async () => {
         const values = await allValuesFrom(
             observeAnnotations<Range>(
-                Observable.of(
+                observableOfTimedSequence(
                     [
                         {
                             uri: 'a',
@@ -362,6 +362,7 @@ describe('observeAnnotations', () => {
                             settings: {},
                         },
                     ],
+                    0,
                     [
                         {
                             uri: 'a',

lib/client/src/api.ts

@@ -16,9 +16,9 @@ import {
     combineLatest,
     defer,
     distinctUntilChanged,
-    mergeMap,
     promiseOrObservableToObservable,
     startWith,
+    switchMap,
     tap,
 } from './misc/observable.js'
 import type { ProviderClient } from './providerClient/createProviderClient.js'
@@ -78,7 +78,7 @@ function observeProviderCall<R>(
     const EMIT_PARTIAL_SENTINEL: 'emit-partial-sentinel' = {} as any
 
     return providerClients.pipe(
-        mergeMap(providerClients =>
+        switchMap(providerClients =>
             providerClients && providerClients.length > 0
                 ? combineLatest(
                       providerClients.map(({ uri, providerClient, settings }) =>

lib/client/src/client/client.ts

@@ -35,9 +35,9 @@ import {
     concatMap,
     distinctUntilChanged,
     firstValueFrom,
-    mergeMap,
     promiseOrObservableToObservable,
     shareReplay,
+    switchMap,
     take,
     timer,
 } from '../misc/observable.js'
@@ -288,7 +288,7 @@ export function createClient<R extends Range>(env: ClientEnv<R>): Client<R> {
                 }),
             )
             .pipe(
-                mergeMap(([configuration, providers]) =>
+                switchMap(([configuration, providers]) =>
                     configuration.providers.length > 0
                         ? combineLatest(
                               configuration.providers.map(({ providerUri, settings }) =>

lib/client/src/misc/observable.test.ts

@@ -1,4 +1,4 @@
-import { Observable } from 'observable-fns'
+import { Observable, Subject } from 'observable-fns'
 import { afterEach, describe, expect, test, vi } from 'vitest'
 import {
     type ObservableValue,
@@ -7,12 +7,14 @@ import {
     distinctUntilChanged,
     firstValueFrom,
     fromVSCodeEvent,
+    isEqualJSON,
     memoizeLastValue,
     observableOfSequence,
     observableOfTimedSequence,
     promiseFactoryToObservable,
     readValuesFrom,
     shareReplay,
+    switchMap,
 } from './observable.js'
 
 describe('firstValueFrom', () => {
@@ -285,3 +287,151 @@ describe('shareReplay', () => {
         expect(called).toBe(1) // but the observable was only heated up once
     })
 })
+
+describe('switchMap', () => {
+    test('switches to new inner observable when source emits', async () => {
+        vi.useFakeTimers()
+        const source = new Subject<string>()
+        const result = source.pipe(switchMap(c => observableOfTimedSequence(10, `${c}-1`, 10, `${c}-2`)))
+        const { values, done } = readValuesFrom(result)
+
+        source.next('a')
+        await vi.advanceTimersByTimeAsync(15)
+        expect(values).toEqual<typeof values>(['a-1'])
+        values.length = 0
+
+        source.next('b')
+        await vi.advanceTimersByTimeAsync(15)
+        expect(values).toEqual<typeof values>(['b-1'])
+        values.length = 0
+
+        await vi.advanceTimersByTimeAsync(10)
+        expect(values).toEqual<typeof values>(['b-2'])
+        values.length = 0
+
+        source.complete()
+        await done
+        expect(values).toEqual<typeof values>([])
+    })
+
+    test('unsubscribes from previous inner observable', async () => {
+        vi.useFakeTimers()
+        const innerSubject1 = new Subject<string>()
+        const innerSubject2 = new Subject<string>()
+        const source = new Subject<number>()
+        const result = source.pipe(switchMap(x => (x === 1 ? innerSubject1 : innerSubject2)))
+        const { values, done } = readValuesFrom(result)
+
+        source.next(1)
+        innerSubject1.next('a')
+        expect(values).toEqual(['a'])
+
+        source.next(2)
+        innerSubject1.next('b') // This should be ignored
+        innerSubject2.next('c')
+        expect(values).toEqual(['a', 'c'])
+
+        source.complete()
+        innerSubject2.complete()
+        await done
+        expect(values).toEqual(['a', 'c'])
+    })
+
+    test('handles errors from source observable', async () => {
+        vi.useFakeTimers()
+        const source = new Subject<number>()
+        const result = source.pipe(switchMap(x => observableOfSequence(x * 10)))
+        const { values, done } = readValuesFrom(result)
+
+        source.next(1)
+        await vi.advanceTimersByTimeAsync(10)
+        source.next(2)
+        await vi.advanceTimersByTimeAsync(10)
+        source.error(new Error('Source error'))
+
+        await expect(done).rejects.toThrow('Source error')
+        expect(values).toEqual([10, 20])
+    })
+
+    test('handles errors from inner observable', async () => {
+        vi.useFakeTimers()
+        const source = new Subject<number>()
+        const result = source.pipe(
+            switchMap(x => {
+                if (x === 2) {
+                    return new Observable(observer => observer.error(new Error('Inner error')))
+                }
+                return observableOfSequence(x * 10)
+            }),
+        )
+        const { values, done } = readValuesFrom(result)
+        done.catch(() => {})
+
+        source.next(1)
+        await vi.advanceTimersByTimeAsync(10)
+        source.next(2)
+        await vi.advanceTimersByTimeAsync(10)
+
+        await expect(done).rejects.toThrow('Inner error')
+        expect(values).toEqual([10])
+    })
+
+    test('completes when source completes and last inner observable completes', async () => {
+        vi.useFakeTimers()
+        const source = new Subject<string>()
+        const result = source.pipe(switchMap(c => observableOfTimedSequence(10, `${c}-1`, 10, `${c}-2`)))
+        const { values, done } = readValuesFrom(result)
+
+        source.next('a')
+        await vi.advanceTimersByTimeAsync(20)
+        source.complete()
+
+        await done
+        expect(values).toEqual(['a-1', 'a-2'])
+    })
+})
+
+describe('isEqualJSON', () => {
+    test('compares objects deeply', () => {
+        const obj1 = { a: 1, b: { c: 2 } }
+        const obj2 = { a: 1, b: { c: 2 } }
+        const obj3 = { a: 1, b: { c: 3 } }
+        const obj4 = { a: 1, b: { x: 3 } }
+
+        expect(isEqualJSON(obj1, obj2)).toBe(true)
+        expect(isEqualJSON(obj1, obj3)).toBe(false)
+        expect(isEqualJSON<unknown>(obj3, obj4)).toBe(false)
+    })
+
+    test('handles arrays', () => {
+        const arr1 = [1, 2, [3, 4]]
+        const arr2 = [1, 2, [3, 4]]
+        const arr3 = [1, 2, [3, 5]]
+
+        expect(isEqualJSON(arr1, arr2)).toBe(true)
+        expect(isEqualJSON(arr1, arr3)).toBe(false)
+
+        expect(isEqualJSON(['a'], { '0': 'a' })).toBe(false)
+        expect(isEqualJSON([undefined], { '0': undefined })).toBe(false)
+        expect(isEqualJSON([undefined], [null])).toBe(false)
+    })
+
+    test('handles null and undefined', () => {
+        expect(isEqualJSON(null, null)).toBe(true)
+        expect(isEqualJSON(undefined, undefined)).toBe(true)
+        expect(isEqualJSON(null, undefined)).toBe(false)
+    })
+
+    test('handles primitive types', () => {
+        expect(isEqualJSON(1, 1)).toBe(true)
+        expect(isEqualJSON('test', 'test')).toBe(true)
+        expect(isEqualJSON(true, true)).toBe(true)
+        expect(isEqualJSON<unknown>(1, '1')).toBe(false)
+    })
+
+    test('handles unset properties', () => {
+        expect(isEqualJSON({ a: undefined }, {})).toBe(true)
+        expect(isEqualJSON({}, { b: undefined })).toBe(true)
+        expect(isEqualJSON({}, { c: 123 })).toBe(false)
+    })
+})