fix(ui): fix use observable (#1719)

* fix: fix package.json conflicting

* fix: fix link

* fix: fix lock file

packages/ui/package.json

@@ -85,6 +85,7 @@
     },
     "devDependencies": {
         "@testing-library/react": "^14.2.1",
+        "@testing-library/react-hooks": "^8.0.1",
         "@types/react": "^18.2.72",
         "@types/react-dom": "^18.2.22",
         "@types/react-transition-group": "^4.4.10",

packages/ui/src/components/font-size/FontSize.tsx

@@ -16,15 +16,14 @@
 
 import { InputNumber } from '@univerjs/design';
 import React, { useMemo, useState } from 'react';
-import { Observable } from 'rxjs';
 
 import { useObservable } from '../hooks/observable';
 import styles from './index.module.less';
 import type { IFontSizeProps } from './interface';
 
 export const FontSize = (props: IFontSizeProps) => {
     const { value, min, max, onChange, disabled$ } = props;
-    const disabled = useObservable(disabled$ ?? new Observable<boolean>((sub) => sub.next(false)));
+    const disabled = useObservable(disabled$);
     const [realValue, setRealValue] = useState<number>(Number(value ?? 0));
 
     const _value = useMemo(() => Number(value ?? realValue), [value]);

packages/ui/src/components/hooks/__tests__/observable.spec.ts

@@ -0,0 +1,101 @@
+/**
+ * Copyright 2023-present DreamNum Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { describe, expect, it } from 'vitest';
+import { act, renderHook } from '@testing-library/react-hooks';
+import { of, Subject } from 'rxjs';
+import type { Observable } from 'rxjs';
+
+import { useState } from 'react';
+import { useObservable } from '../observable';
+
+// New to testing React Hooks? You can refer to https://mayashavin.com/articles/test-react-hooks-with-vitest .
+
+describe('test "useObservable"', () => {
+    it('should return undefined when no initial value is provided', () => {
+        const observable: Observable<boolean> | undefined = undefined;
+
+        const { result } = renderHook(() => useObservable<boolean>(observable));
+        expect(result.current).toBeUndefined();
+    });
+
+    it('should return the initial value when provided', () => {
+        const observable: Observable<boolean> | undefined = undefined;
+
+        const { result } = renderHook(() => useObservable<boolean>(observable, true));
+        expect(result.current).toBeTruthy();
+    });
+
+    it('should return the initial value when provided synchronously', () => {
+        const observable: Observable<boolean> = of(true);
+
+        const { result } = renderHook(() => useObservable<boolean>(observable));
+        expect(result.current).toBeTruthy();
+    });
+
+    function useTestUseObservableBed() {
+        const observable = new Subject<boolean>();
+        const result = useObservable(observable, undefined, false);
+
+        return {
+            observable,
+            result,
+        };
+    }
+
+    it('should emit new value when observable emits', () => {
+        const { result } = renderHook(() => useTestUseObservableBed());
+
+        expect(result.current.result).toBeUndefined();
+
+        act(() => result.current.observable.next(true));
+        expect(result.current.result).toBeTruthy();
+
+        act(() => result.current.observable.next(false));
+        expect(result.current.result).toBeFalsy();
+    });
+
+    function useTestSwitchObservableBed() {
+        const [observable, setObservable] = useState<Observable<boolean> | undefined>(undefined);
+        const result = useObservable(observable);
+
+        return {
+            result,
+            observable,
+            setObservable,
+        };
+    }
+
+    it('should emit when passing new observable to the hook', () => {
+        const { result } = renderHook(() => useTestSwitchObservableBed());
+
+        expect(result.current.result).toBeUndefined();
+
+        act(() => result.current.setObservable(of(true)));
+        expect(result.current.result).toBeTruthy();
+
+        act(() => result.current.setObservable(of(false)));
+        expect(result.current.result).toBeFalsy();
+    });
+
+    it('should support a callback function returns an observable', () => {
+        // const { result } = renderHook(() => useObservable(() => of(true)));
+        // This line above would cause infinite look. Pass `deps` to fix the problem.
+        const { result } = renderHook(() => useObservable(() => of(true), undefined, true, []));
+
+        expect(result.current).toBeTruthy();
+    });
+});

packages/ui/src/components/hooks/observable.ts

@@ -14,7 +14,9 @@
  * limitations under the License.
  */
 
-import type React from 'react';
+/* eslint-disable ts/no-explicit-any */
+
+import type { Nullable } from '@univerjs/core';
 import { useEffect, useRef, useState } from 'react';
 import type { Observable, Subscription } from 'rxjs';
 
@@ -36,27 +38,39 @@ function showArrayNotEqual(arr1: unknown[], arr2: unknown[]): boolean {
     return arr1.some((value, index) => value !== arr2[index]);
 }
 
-export function useObservable<T>(observable: Observable<T>, defaultValue?: T): T | undefined;
-export function useObservable<T>(observable: Observable<T>, defaultValue?: T, shouldHaveSyncValue?: true): T;
-export function useObservable<T>(
-    observable: ObservableOrFn<T>,
-    defaultValue?: T,
-    shouldHaveSyncValue?: false,
-    deps?: any[]
-): T | undefined;
-export function useObservable<T>(
-    observable: ObservableOrFn<T>,
-    defaultValue?: T,
-    shouldHaveSyncValue?: boolean,
-    deps?: any[]
-): T | undefined {
+export function useObservable<T>(observable: ObservableOrFn<T>, defaultValue: T | undefined, shouldHaveSyncValue?: true): T;
+export function useObservable<T>(observable: Nullable<ObservableOrFn<T>>, defaultValue: T): T;
+export function useObservable<T>(observable: Nullable<ObservableOrFn<T>>, defaultValue?: undefined): T | undefined;
+export function useObservable<T>(observable: Nullable<ObservableOrFn<T>>, defaultValue?: T, shouldHaveSyncValue?: true, deps?: any[]): T | undefined;
+export function useObservable<T>(observable: Nullable<ObservableOrFn<T>>, defaultValue?: T, shouldHaveSyncValue?: boolean, deps?: any[]): T | undefined;
+/**
+ * A hook to subscribe to an observable and get the latest value.
+ *
+ * @param observable The observable to subscribe to.
+ * @param defaultValue When the observable would not emit any value, the default value would be returned.
+ * @param shouldHaveSyncValue If true, the observable should emit a value synchronously.
+ * @param deps The dependencies to trigger a re-subscription.
+ */
+export function useObservable<T>(observable: Nullable<ObservableOrFn<T>>, defaultValue?: T, shouldHaveSyncValue?: boolean, deps?: any[]): T | undefined {
     const observableRef = useRef<Observable<T> | null>(null);
     const subscriptionRef = useRef<Subscription | null>(null);
     const depsRef = useRef<any[] | undefined>(deps ?? undefined);
     const initializedRef = useRef<boolean>(false);
 
-    let setValue: React.Dispatch<React.SetStateAction<T | undefined>>;
-    let innerDefaultValue: T | undefined;
+    const [value, setValue] = useState<T | undefined>(() => {
+        let innerDefaultValue: T | undefined = defaultValue;
+
+        if (observable) {
+            const sub = unwrap(observable).subscribe((value) => {
+                initializedRef.current = true;
+                innerDefaultValue = value;
+            });
+
+            sub.unsubscribe();
+        }
+
+        return innerDefaultValue;
+    });
 
     const shouldResubscribe = (() => {
         if (typeof depsRef.current !== 'undefined') {
@@ -72,30 +86,19 @@ export function useObservable<T>(
         return observableRef.current !== observable;
     })();
 
-    if (!subscriptionRef.current || shouldResubscribe) {
+    if ((!subscriptionRef.current || shouldResubscribe) && observable) {
         observableRef.current = unwrap(observable);
         subscriptionRef.current?.unsubscribe();
         subscriptionRef.current = observableRef.current.subscribe((value) => {
-            if (setValue) {
-                setValue(value);
-            } else {
-                innerDefaultValue = value;
-                initializedRef.current = true;
-            }
+            setValue(value);
         });
     }
 
     if (shouldHaveSyncValue && !initializedRef.current) {
         throw new Error('[useObservable]: expect shouldHaveSyncValue but not getting a sync value!');
     }
 
-    const s = useState<T | undefined>(innerDefaultValue || defaultValue);
-    const value = s[0];
-    setValue = s[1];
-
-    useEffect(() => {
-        return () => subscriptionRef.current?.unsubscribe();
-    }, []);
+    useEffect(() => () => subscriptionRef.current?.unsubscribe(), []);
 
     return value;
 }

packages/ui/src/components/menu/Menu.tsx

@@ -25,7 +25,7 @@ import { CheckMarkSingle, MoreSingle } from '@univerjs/icons';
 import { useDependency } from '@wendellhu/redi/react-bindings';
 import clsx from 'clsx';
 import React, { useState } from 'react';
-import { isObservable, of } from 'rxjs';
+import { isObservable } from 'rxjs';
 
 import type {
     IDisplayMenuItem,
@@ -180,9 +180,9 @@ function MenuItem({ menuItem, onClick }: IMenuItemProps) {
 
     const menuItems = menuItem.id ? menuService.getMenuItems(menuItem.id) : [];
 
-    const disabled = useObservable<boolean>(menuItem.disabled$ || of(false), false, true);
-    const hidden = useObservable(menuItem.hidden$ || of(false), false, true);
-    const value = useObservable<MenuItemDefaultValueType>(menuItem.value$ || of(undefined), undefined, true);
+    const disabled = useObservable<boolean>(menuItem.disabled$, false);
+    const hidden = useObservable(menuItem.hidden$, false);
+    const value = useObservable<MenuItemDefaultValueType>(menuItem.value$);
     const [inputValue, setInputValue] = useState(value);
 
     /**
@@ -217,12 +217,8 @@ function MenuItem({ menuItem, onClick }: IMenuItemProps) {
     const renderSelectorType = () => {
         const item = menuItem as IDisplayMenuItem<IMenuSelectorItem>;
 
-        let selections: IValueOption[];
-        if (isObservable(item.selections)) {
-            selections = useObservable<IValueOption[]>(item.selections || of([]), [], true);
-        } else {
-            selections = item.selections || [];
-        }
+        const selectionsFromObservable = useObservable(isObservable(item.selections) ? item.selections : undefined);
+        const selections = selectionsFromObservable ?? (item.selections as IValueOption[] | undefined) ?? [];
 
         if (selections.length > 0) {
             return (

packages/ui/src/views/components/doc-bars/ToolbarItem.tsx

@@ -110,12 +110,9 @@ export const ToolbarItem = forwardRef((props: IDisplayMenuItem<IMenuItem>, ref:
     const { selections } = props as IDisplayMenuItem<IMenuSelectorItem>;
 
     const options = selections as IValueOption[];
-    let iconToDisplay = icon;
-    if (isObservable(icon)) {
-        iconToDisplay = useObservable(icon, undefined, true);
-    } else {
-        iconToDisplay = options?.find((o) => o.value === value)?.icon ?? icon;
-    }
+
+    const iconFromObservable = useObservable(isObservable(icon) ? icon : undefined);
+    const iconToDisplay = iconFromObservable ?? options?.find((o) => o.value === value)?.icon ?? icon;
 
     function renderSelectorType(menuType: MenuItemType) {
         function handleSelect(option: IValueOption) {

packages/ui/src/views/hooks/active.ts

@@ -17,7 +17,6 @@
 import type { Nullable, Workbook, Worksheet } from '@univerjs/core';
 import { IUniverInstanceService } from '@univerjs/core';
 import { useDependency } from '@wendellhu/redi/react-bindings';
-import { of } from 'rxjs';
 
 import { useObservable } from '../../components/hooks/observable';
 
@@ -35,6 +34,6 @@ export function useActiveWorkbook(): Nullable<Workbook> {
  */
 export function useActiveWorksheet(): Nullable<Worksheet> {
     const activeWorkbook = useActiveWorkbook();
-    const activeWorksheet = useObservable(activeWorkbook ? activeWorkbook?.activeSheet$ : of(null));
+    const activeWorksheet = useObservable(activeWorkbook?.activeSheet$, null);
     return activeWorksheet;
 }