diff --git a/packages/ui/package.json b/packages/ui/package.json index 9d4c4977f8f..784a202d097 100644 --- a/packages/ui/package.json +++ b/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", diff --git a/packages/ui/src/components/font-size/FontSize.tsx b/packages/ui/src/components/font-size/FontSize.tsx index 580072b2964..0052e980037 100644 --- a/packages/ui/src/components/font-size/FontSize.tsx +++ b/packages/ui/src/components/font-size/FontSize.tsx @@ -16,7 +16,6 @@ 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'; @@ -24,7 +23,7 @@ import type { IFontSizeProps } from './interface'; export const FontSize = (props: IFontSizeProps) => { const { value, min, max, onChange, disabled$ } = props; - const disabled = useObservable(disabled$ ?? new Observable((sub) => sub.next(false))); + const disabled = useObservable(disabled$); const [realValue, setRealValue] = useState(Number(value ?? 0)); const _value = useMemo(() => Number(value ?? realValue), [value]); diff --git a/packages/ui/src/components/hooks/__tests__/observable.spec.ts b/packages/ui/src/components/hooks/__tests__/observable.spec.ts new file mode 100644 index 00000000000..626b46ed00a --- /dev/null +++ b/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 | undefined = undefined; + + const { result } = renderHook(() => useObservable(observable)); + expect(result.current).toBeUndefined(); + }); + + it('should return the initial value when provided', () => { + const observable: Observable | undefined = undefined; + + const { result } = renderHook(() => useObservable(observable, true)); + expect(result.current).toBeTruthy(); + }); + + it('should return the initial value when provided synchronously', () => { + const observable: Observable = of(true); + + const { result } = renderHook(() => useObservable(observable)); + expect(result.current).toBeTruthy(); + }); + + function useTestUseObservableBed() { + const observable = new Subject(); + 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 | 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(); + }); +}); diff --git a/packages/ui/src/components/hooks/observable.ts b/packages/ui/src/components/hooks/observable.ts index 960a4ced9cb..ae2d96b7c48 100644 --- a/packages/ui/src/components/hooks/observable.ts +++ b/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(observable: Observable, defaultValue?: T): T | undefined; -export function useObservable(observable: Observable, defaultValue?: T, shouldHaveSyncValue?: true): T; -export function useObservable( - observable: ObservableOrFn, - defaultValue?: T, - shouldHaveSyncValue?: false, - deps?: any[] -): T | undefined; -export function useObservable( - observable: ObservableOrFn, - defaultValue?: T, - shouldHaveSyncValue?: boolean, - deps?: any[] -): T | undefined { +export function useObservable(observable: ObservableOrFn, defaultValue: T | undefined, shouldHaveSyncValue?: true): T; +export function useObservable(observable: Nullable>, defaultValue: T): T; +export function useObservable(observable: Nullable>, defaultValue?: undefined): T | undefined; +export function useObservable(observable: Nullable>, defaultValue?: T, shouldHaveSyncValue?: true, deps?: any[]): T | undefined; +export function useObservable(observable: Nullable>, 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(observable: Nullable>, defaultValue?: T, shouldHaveSyncValue?: boolean, deps?: any[]): T | undefined { const observableRef = useRef | null>(null); const subscriptionRef = useRef(null); const depsRef = useRef(deps ?? undefined); const initializedRef = useRef(false); - let setValue: React.Dispatch>; - let innerDefaultValue: T | undefined; + const [value, setValue] = useState(() => { + 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,16 +86,11 @@ export function useObservable( 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); }); } @@ -89,13 +98,7 @@ export function useObservable( throw new Error('[useObservable]: expect shouldHaveSyncValue but not getting a sync value!'); } - const s = useState(innerDefaultValue || defaultValue); - const value = s[0]; - setValue = s[1]; - - useEffect(() => { - return () => subscriptionRef.current?.unsubscribe(); - }, []); + useEffect(() => () => subscriptionRef.current?.unsubscribe(), []); return value; } diff --git a/packages/ui/src/components/menu/Menu.tsx b/packages/ui/src/components/menu/Menu.tsx index 37f9237b937..5db072d0059 100644 --- a/packages/ui/src/components/menu/Menu.tsx +++ b/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(menuItem.disabled$ || of(false), false, true); - const hidden = useObservable(menuItem.hidden$ || of(false), false, true); - const value = useObservable(menuItem.value$ || of(undefined), undefined, true); + const disabled = useObservable(menuItem.disabled$, false); + const hidden = useObservable(menuItem.hidden$, false); + const value = useObservable(menuItem.value$); const [inputValue, setInputValue] = useState(value); /** @@ -217,12 +217,8 @@ function MenuItem({ menuItem, onClick }: IMenuItemProps) { const renderSelectorType = () => { const item = menuItem as IDisplayMenuItem; - let selections: IValueOption[]; - if (isObservable(item.selections)) { - selections = useObservable(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 ( diff --git a/packages/ui/src/views/components/doc-bars/ToolbarItem.tsx b/packages/ui/src/views/components/doc-bars/ToolbarItem.tsx index 8a90a63dfe5..4f402706371 100644 --- a/packages/ui/src/views/components/doc-bars/ToolbarItem.tsx +++ b/packages/ui/src/views/components/doc-bars/ToolbarItem.tsx @@ -110,12 +110,9 @@ export const ToolbarItem = forwardRef((props: IDisplayMenuItem, ref: const { selections } = props as IDisplayMenuItem; 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) { diff --git a/packages/ui/src/views/hooks/active.ts b/packages/ui/src/views/hooks/active.ts index 7f7a20c7dad..f7a42b76d1e 100644 --- a/packages/ui/src/views/hooks/active.ts +++ b/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 { */ export function useActiveWorksheet(): Nullable { const activeWorkbook = useActiveWorkbook(); - const activeWorksheet = useObservable(activeWorkbook ? activeWorkbook?.activeSheet$ : of(null)); + const activeWorksheet = useObservable(activeWorkbook?.activeSheet$, null); return activeWorksheet; } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 78db356bc6c..194fe313dfd 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1131,6 +1131,9 @@ importers: '@testing-library/react': specifier: ^14.2.1 version: 14.2.1(react-dom@18.2.0)(react@18.2.0) + '@testing-library/react-hooks': + specifier: ^8.0.1 + version: 8.0.1(@types/react@18.2.72)(react-dom@18.2.0)(react@18.2.0) '@types/react': specifier: ^18.2.72 version: 18.2.72 @@ -5140,6 +5143,29 @@ packages: redent: 3.0.0 dev: false + /@testing-library/react-hooks@8.0.1(@types/react@18.2.72)(react-dom@18.2.0)(react@18.2.0): + resolution: {integrity: sha512-Aqhl2IVmLt8IovEVarNDFuJDVWVvhnr9/GCU6UUnrYXwgDFF9h2L2o2P9KBni1AST5sT6riAyoukFLyjQUgD/g==} + engines: {node: '>=12'} + peerDependencies: + '@types/react': ^16.9.0 || ^17.0.0 + react: ^16.9.0 || ^17.0.0 + react-dom: ^16.9.0 || ^17.0.0 + react-test-renderer: ^16.9.0 || ^17.0.0 + peerDependenciesMeta: + '@types/react': + optional: true + react-dom: + optional: true + react-test-renderer: + optional: true + dependencies: + '@babel/runtime': 7.23.9 + '@types/react': 18.2.72 + react: 18.2.0 + react-dom: 18.2.0(react@18.2.0) + react-error-boundary: 3.1.4(react@18.2.0) + dev: true + /@testing-library/react@14.2.1(react-dom@18.2.0)(react@18.2.0): resolution: {integrity: sha512-sGdjws32ai5TLerhvzThYFbpnF9XtL65Cjf+gB0Dhr29BGqK+mAeN7SURSdu+eqgET4ANcWoC7FQpkaiGvBr+A==} engines: {node: '>=14'} @@ -12595,6 +12621,16 @@ packages: react-dom: 18.2.0(react@18.2.0) react-is: 18.1.0 + /react-error-boundary@3.1.4(react@18.2.0): + resolution: {integrity: sha512-uM9uPzZJTF6wRQORmSrvOIgt4lJ9MC1sNgEOj2XGsDTRE4kmpWxg7ENK9EWNKJRMAOY9z0MuF4yIfl6gp4sotA==} + engines: {node: '>=10', npm: '>=6'} + peerDependencies: + react: '>=16.13.1' + dependencies: + '@babel/runtime': 7.23.9 + react: 18.2.0 + dev: true + /react-is@16.13.1: resolution: {integrity: sha512-24e6ynE2H+OKt4kqsOvNd8kBpV65zoxbA4BVsEOB3ARVWQki/DHzaUoC5KuON/BiccDaCCTZBuOcfZs70kR8bQ==}