From 45c78a0ef57933a4bce234ed75f50eed00243e91 Mon Sep 17 00:00:00 2001 From: Mike Bender Date: Tue, 4 Jun 2024 15:31:55 -0400 Subject: [PATCH] feat: DH-16737 Add ObjectManager, `useWidget` hook (#2030) - Hook for loading a widget that makes it easy to use - `ObjectManager` context allows for implementation to handle loading the object - On Core side, we have a very simple `ObjectManager`, as we just have one connection - On Enterprise side, the `ObjectManager` provided to the context manager will need to handle fetching from queries, and will be able to handle more scenarios (such as when a query is restarting) - Use with https://github.com/deephaven/deephaven-plugins/pull/502 --- package-lock.json | 4 +- .../src/components/ConnectionBootstrap.tsx | 27 ++- packages/jsapi-bootstrap/package.json | 3 +- packages/jsapi-bootstrap/src/index.ts | 2 + .../src/useObjectFetch.test.tsx | 35 +++ .../jsapi-bootstrap/src/useObjectFetch.ts | 91 ++++++++ .../jsapi-bootstrap/src/useWidget.test.tsx | 213 ++++++++++++++++++ packages/jsapi-bootstrap/src/useWidget.ts | 103 +++++++++ packages/jsapi-bootstrap/tsconfig.json | 3 +- 9 files changed, 476 insertions(+), 5 deletions(-) create mode 100644 packages/jsapi-bootstrap/src/useObjectFetch.test.tsx create mode 100644 packages/jsapi-bootstrap/src/useObjectFetch.ts create mode 100644 packages/jsapi-bootstrap/src/useWidget.test.tsx create mode 100644 packages/jsapi-bootstrap/src/useWidget.ts diff --git a/package-lock.json b/package-lock.json index 4c7252e500..ea43ddf771 100644 --- a/package-lock.json +++ b/package-lock.json @@ -29372,7 +29372,8 @@ "@deephaven/components": "file:../components", "@deephaven/jsapi-types": "1.0.0-dev0.34.0", "@deephaven/log": "file:../log", - "@deephaven/react-hooks": "file:../react-hooks" + "@deephaven/react-hooks": "file:../react-hooks", + "@deephaven/utils": "file:../utils" }, "devDependencies": { "react": "^17.x" @@ -31600,6 +31601,7 @@ "@deephaven/jsapi-types": "1.0.0-dev0.34.0", "@deephaven/log": "file:../log", "@deephaven/react-hooks": "file:../react-hooks", + "@deephaven/utils": "file:../utils", "react": "^17.x" } }, diff --git a/packages/app-utils/src/components/ConnectionBootstrap.tsx b/packages/app-utils/src/components/ConnectionBootstrap.tsx index c841a6debe..9ed32b4640 100644 --- a/packages/app-utils/src/components/ConnectionBootstrap.tsx +++ b/packages/app-utils/src/components/ConnectionBootstrap.tsx @@ -1,7 +1,9 @@ -import React, { useCallback, useEffect, useState } from 'react'; +import React, { useCallback, useEffect, useMemo, useState } from 'react'; import { LoadingOverlay } from '@deephaven/components'; import { ObjectFetcherContext, + ObjectFetchManager, + ObjectFetchManagerContext, sanitizeVariableDescriptor, useApi, useClient, @@ -31,6 +33,7 @@ export function ConnectionBootstrap({ const client = useClient(); const [error, setError] = useState(); const [connection, setConnection] = useState(); + useEffect( function initConnection() { let isCanceled = false; @@ -83,6 +86,24 @@ export function ConnectionBootstrap({ [connection] ); + /** We don't really need to do anything fancy in Core to manage an object, just fetch it */ + const objectManager: ObjectFetchManager = useMemo( + () => ({ + subscribe: (descriptor, onUpdate) => { + // We send an update with the fetch right away + onUpdate({ + fetch: () => objectFetcher(descriptor), + status: 'ready', + }); + return () => { + // no-op + // For Core, if the server dies then we can't reconnect anyway, so no need to bother listening for subscription or cleaning up + }; + }, + }), + [objectFetcher] + ); + if (connection == null || error != null) { return ( - {children} + + {children} + ); diff --git a/packages/jsapi-bootstrap/package.json b/packages/jsapi-bootstrap/package.json index 4dea5bf30a..4a43e30bcf 100644 --- a/packages/jsapi-bootstrap/package.json +++ b/packages/jsapi-bootstrap/package.json @@ -25,7 +25,8 @@ "@deephaven/components": "file:../components", "@deephaven/jsapi-types": "1.0.0-dev0.34.0", "@deephaven/log": "file:../log", - "@deephaven/react-hooks": "file:../react-hooks" + "@deephaven/react-hooks": "file:../react-hooks", + "@deephaven/utils": "file:../utils" }, "devDependencies": { "react": "^17.x" diff --git a/packages/jsapi-bootstrap/src/index.ts b/packages/jsapi-bootstrap/src/index.ts index 23ce282865..a016f8a2bf 100644 --- a/packages/jsapi-bootstrap/src/index.ts +++ b/packages/jsapi-bootstrap/src/index.ts @@ -4,4 +4,6 @@ export * from './DeferredApiBootstrap'; export * from './useApi'; export * from './useClient'; export * from './useDeferredApi'; +export * from './useObjectFetch'; export * from './useObjectFetcher'; +export * from './useWidget'; diff --git a/packages/jsapi-bootstrap/src/useObjectFetch.test.tsx b/packages/jsapi-bootstrap/src/useObjectFetch.test.tsx new file mode 100644 index 0000000000..3dd4c1a367 --- /dev/null +++ b/packages/jsapi-bootstrap/src/useObjectFetch.test.tsx @@ -0,0 +1,35 @@ +import React from 'react'; +import { renderHook } from '@testing-library/react-hooks'; +import { ObjectFetchManagerContext, useObjectFetch } from './useObjectFetch'; + +it('should resolve the objectFetch when in the context', async () => { + const objectFetch = jest.fn(async () => undefined); + const unsubscribe = jest.fn(); + const descriptor = { type: 'type', name: 'name' }; + const subscribe = jest.fn((subscribeDescriptor, onUpdate) => { + expect(subscribeDescriptor).toEqual(descriptor); + onUpdate({ fetch: objectFetch, status: 'ready' }); + return unsubscribe; + }); + const objectManager = { subscribe }; + const wrapper = ({ children }) => ( + + {children} + + ); + + const { result } = renderHook(() => useObjectFetch(descriptor), { wrapper }); + expect(result.current).toEqual({ fetch: objectFetch, status: 'ready' }); + expect(result.error).toBeUndefined(); + expect(objectFetch).not.toHaveBeenCalled(); +}); + +it('should return an error, not throw if objectFetch not available in the context', async () => { + const descriptor = { type: 'type', name: 'name' }; + const { result } = renderHook(() => useObjectFetch(descriptor)); + expect(result.current).toEqual({ + error: expect.any(Error), + status: 'error', + }); + expect(result.error).toBeUndefined(); +}); diff --git a/packages/jsapi-bootstrap/src/useObjectFetch.ts b/packages/jsapi-bootstrap/src/useObjectFetch.ts new file mode 100644 index 0000000000..5330ca5fd3 --- /dev/null +++ b/packages/jsapi-bootstrap/src/useObjectFetch.ts @@ -0,0 +1,91 @@ +import { createContext, useContext, useEffect, useState } from 'react'; +import type { dh } from '@deephaven/jsapi-types'; + +/** Function for unsubscribing from a given subscription */ +export type UnsubscribeFunction = () => void; + +/** Update when the ObjectFetch is still loading */ +export type ObjectFetchLoading = { + status: 'loading'; +}; + +/** Update when the ObjectFetch has errored */ +export type ObjectFetchError = { + error: NonNullable; + status: 'error'; +}; + +/** Update when the object is ready */ +export type ObjectFetchReady = { + fetch: () => Promise; + status: 'ready'; +}; + +/** + * Update with the current `fetch` function and status of the object. + * - If both `fetch` and `error` are `null`, it is still loading the fetcher + * - If `fetch` is not `null`, the object is ready to be fetched + * - If `error` is not `null`, there was an error loading the object + */ +export type ObjectFetchUpdate = + | ObjectFetchLoading + | ObjectFetchError + | ObjectFetchReady; + +export type ObjectFetchUpdateCallback = ( + update: ObjectFetchUpdate +) => void; + +/** ObjectFetchManager for managing a subscription to an object using a VariableDescriptor */ +export type ObjectFetchManager = { + /** + * Subscribe to the fetch function for an object using a variable descriptor. + * It's possible that the fetch function changes over time, due to disconnection/reconnection, starting/stopping of applications that the object may be associated with, etc. + * + * @param descriptor Descriptor object of the object to fetch. Can be extended by a specific implementation to include more details necessary for the ObjectManager. + * @param onUpdate Callback function to be called when the object is updated. + * @returns An unsubscribe function to stop listening for fetch updates and clean up the object. + */ + subscribe: ( + descriptor: dh.ide.VariableDescriptor, + onUpdate: ObjectFetchUpdateCallback + ) => UnsubscribeFunction; +}; + +/** Context for tracking an implementation of the ObjectFetchManager. */ +export const ObjectFetchManagerContext = + createContext(null); + +/** + * Retrieve a `fetch` function for the given variable descriptor. + * + * @param descriptor Descriptor to get the `fetch` function for + * @returns An object with the current `fetch` function, OR an error status set if there was an issue fetching the object. + * Retrying is left up to the ObjectManager implementation used from this context. + */ +export function useObjectFetch( + descriptor: dh.ide.VariableDescriptor +): ObjectFetchUpdate { + const [currentUpdate, setCurrentUpdate] = useState>({ + status: 'loading', + }); + + const objectFetchManager = useContext(ObjectFetchManagerContext); + + useEffect(() => { + if (objectFetchManager == null) { + setCurrentUpdate({ + error: new Error('No ObjectFetchManager available in context'), + status: 'error', + }); + return; + } + // Update to signal we're still loading, if we're not already in a loading state. + setCurrentUpdate(oldUpdate => + oldUpdate.status === 'loading' ? oldUpdate : { status: 'loading' } + ); + return objectFetchManager.subscribe(descriptor, setCurrentUpdate); + }, [descriptor, objectFetchManager]); + + return currentUpdate; +} diff --git a/packages/jsapi-bootstrap/src/useWidget.test.tsx b/packages/jsapi-bootstrap/src/useWidget.test.tsx new file mode 100644 index 0000000000..f6221b04da --- /dev/null +++ b/packages/jsapi-bootstrap/src/useWidget.test.tsx @@ -0,0 +1,213 @@ +import React from 'react'; +import { act, renderHook } from '@testing-library/react-hooks'; +import { dh } from '@deephaven/jsapi-types'; +import { TestUtils } from '@deephaven/utils'; +import { useWidget } from './useWidget'; +import { ObjectFetchManagerContext } from './useObjectFetch'; + +const WIDGET_TYPE = 'OtherWidget'; + +describe('useWidget', () => { + it('should return a widget when available', async () => { + const descriptor: dh.ide.VariableDescriptor = { + type: 'OtherWidget', + name: 'name', + }; + const widget = { close: jest.fn() }; + const fetch = jest.fn(async () => widget); + const objectFetch = { fetch, error: null }; + const subscribe = jest.fn((subscribeDescriptor, onUpdate) => { + expect(subscribeDescriptor).toEqual(descriptor); + onUpdate(objectFetch); + return jest.fn(); + }); + const objectManager = { subscribe }; + const wrapper = ({ children }) => ( + + {children} + + ); + const { result } = renderHook(() => useWidget(descriptor), { wrapper }); + await act(TestUtils.flushPromises); + expect(result.current).toEqual({ widget, error: null }); + expect(fetch).toHaveBeenCalledTimes(1); + }); + + it('should return an error when an error occurs', () => { + const descriptor: dh.ide.VariableDescriptor = { + type: WIDGET_TYPE, + name: 'name', + }; + const error = new Error('Error fetching widget'); + const objectFetch = { error, status: 'error' }; + const subscribe = jest.fn((subscribeDescriptor, onUpdate) => { + expect(subscribeDescriptor).toEqual(descriptor); + onUpdate(objectFetch); + return jest.fn(); + }); + const objectManager = { subscribe }; + const wrapper = ({ children }) => ( + + {children} + + ); + + const { result } = renderHook(() => useWidget(descriptor), { wrapper }); + + expect(result.current).toEqual({ widget: null, error }); + }); + + it('should return null when still loading', () => { + const descriptor = { type: WIDGET_TYPE, name: 'name' }; + const objectFetch = { status: 'loading' }; + const subscribe = jest.fn((_, onUpdate) => { + onUpdate(objectFetch); + return jest.fn(); + }); + const objectManager = { subscribe }; + const wrapper = ({ children }) => ( + + {children} + + ); + const { result } = renderHook(() => useWidget(descriptor), { wrapper }); + + expect(result.current).toEqual({ widget: null, error: null }); + }); + + it('should close the widget and exported objects when cancelled', async () => { + const descriptor = { type: WIDGET_TYPE, name: 'name' }; + const widget: dh.Widget = TestUtils.createMockProxy({ + close: jest.fn(), + exportedObjects: [ + TestUtils.createMockProxy({ + close: jest.fn(), + }), + TestUtils.createMockProxy({ + close: jest.fn(), + }), + ], + }); + const fetch = jest.fn(async () => widget); + const objectFetch = { fetch, error: null }; + const subscribe = jest.fn((_, onUpdate) => { + onUpdate(objectFetch); + return jest.fn(); + }); + const objectManager = { subscribe }; + const wrapper = ({ children }) => ( + + {children} + + ); + const { result, unmount } = renderHook(() => useWidget(descriptor), { + wrapper, + }); + expect(widget.close).not.toHaveBeenCalled(); + expect(widget.exportedObjects[0].close).not.toHaveBeenCalled(); + expect(widget.exportedObjects[1].close).not.toHaveBeenCalled(); + + expect(result.current).toEqual({ widget: null, error: null }); + + // Unmount before flushing the promise + unmount(); + await act(TestUtils.flushPromises); + expect(widget.close).toHaveBeenCalledTimes(1); + expect(widget.exportedObjects[0].close).toHaveBeenCalledTimes(1); + expect(widget.exportedObjects[1].close).toHaveBeenCalledTimes(1); + }); + + it('should not close the widget if it is returned before unmount', async () => { + const descriptor = { type: WIDGET_TYPE, name: 'name' }; + const widget: dh.Widget = TestUtils.createMockProxy({ + close: jest.fn(), + exportedObjects: [ + TestUtils.createMockProxy({ + close: jest.fn(), + }), + TestUtils.createMockProxy({ + close: jest.fn(), + }), + ], + }); + const fetch = jest.fn(async () => widget); + const objectFetch = { fetch, error: null }; + const subscribe = jest.fn((_, onUpdate) => { + onUpdate(objectFetch); + return jest.fn(); + }); + const objectManager = { subscribe }; + const wrapper = ({ children }) => ( + + {children} + + ); + const { result, unmount } = renderHook(() => useWidget(descriptor), { + wrapper, + }); + + expect(result.current).toEqual({ widget: null, error: null }); + await act(TestUtils.flushPromises); + unmount(); + expect(widget.close).not.toHaveBeenCalled(); + expect(widget.exportedObjects[0].close).not.toHaveBeenCalled(); + expect(widget.exportedObjects[1].close).not.toHaveBeenCalled(); + }); + + it('should handle a Table being fetched', async () => { + const descriptor: dh.ide.VariableDescriptor = { + type: 'Table', + name: 'name', + }; + const table = TestUtils.createMockProxy({ close: jest.fn() }); + const fetch = jest.fn(async () => table); + const objectFetch = { fetch, error: null }; + const subscribe = jest.fn((subscribeDescriptor, onUpdate) => { + expect(subscribeDescriptor).toEqual(descriptor); + onUpdate(objectFetch); + return jest.fn(); + }); + const objectManager = { subscribe }; + const wrapper = ({ children }) => ( + + {children} + + ); + const { result, unmount } = renderHook(() => useWidget(descriptor), { + wrapper, + }); + await act(TestUtils.flushPromises); + expect(result.current).toEqual({ widget: table, error: null }); + expect(fetch).toHaveBeenCalledTimes(1); + unmount(); + + // Shouldn't be called if it was returned before unmount + expect(table.close).not.toHaveBeenCalled(); + }); + + it('should close the Table if unmounted before the fetch is done', async () => { + const descriptor: dh.ide.VariableDescriptor = { + type: 'Table', + name: 'name', + }; + const table = TestUtils.createMockProxy({ close: jest.fn() }); + const fetch = jest.fn(async () => table); + const objectFetch = { fetch, error: null }; + const subscribe = jest.fn((subscribeDescriptor, onUpdate) => { + expect(subscribeDescriptor).toEqual(descriptor); + onUpdate(objectFetch); + return jest.fn(); + }); + const objectManager = { subscribe }; + const wrapper = ({ children }) => ( + + {children} + + ); + const { unmount } = renderHook(() => useWidget(descriptor), { wrapper }); + + unmount(); + await act(TestUtils.flushPromises); + expect(table.close).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/jsapi-bootstrap/src/useWidget.ts b/packages/jsapi-bootstrap/src/useWidget.ts new file mode 100644 index 0000000000..e624245ac2 --- /dev/null +++ b/packages/jsapi-bootstrap/src/useWidget.ts @@ -0,0 +1,103 @@ +import type { dh } from '@deephaven/jsapi-types'; +import Log from '@deephaven/log'; +import { assertNotNull } from '@deephaven/utils'; +import { useEffect, useState } from 'react'; +import { useObjectFetch } from './useObjectFetch'; + +const log = Log.module('useWidget'); + +/** + * Types of widgets that can be fetched with this hook. + */ +type WidgetTypes = + | dh.Table + | dh.TreeTable + | dh.PartitionedTable + | dh.plot.Figure + | dh.Widget; + +/** + * Wrapper object for a widget and error status. Both widget and error will be `null` if it is still loading. + */ +type WidgetWrapper = { + /** Widget object to retrieve */ + widget: T | null; + + /** Error status if there was an issue fetching the widget */ + error: NonNullable | null; +}; + +/** + * Retrieve a widget for the given variable descriptor. Note that if the widget is successfully fetched, ownership of the widget is passed to the consumer and will need to close the object as well. + * @param descriptor Descriptor to get the widget for. Should be stable to avoid infinite re-fetching. + * @returns A WidgetWrapper object that contains the widget or an error status if there was an issue fetching the widget. Will contain nulls if still loading. + */ +export function useWidget( + descriptor: dh.ide.VariableDescriptor +): WidgetWrapper { + const [wrapper, setWrapper] = useState>(() => ({ + widget: null, + error: null, + })); + const objectFetch = useObjectFetch(descriptor); + + useEffect( + function loadWidget() { + log.debug('loadWidget', descriptor); + + const { status } = objectFetch; + + if (status === 'error') { + // We can't fetch if there's an error getting the fetcher, just return an error + setWrapper({ widget: null, error: objectFetch.error }); + return; + } + + if (status === 'loading') { + // Still loading + setWrapper({ widget: null, error: null }); + return; + } + + const { fetch } = objectFetch; + // We should be able to load the widget. Load it asynchronously, and set the widget when it's done. + // If we get cancelled before the fetch is done, we should close the widget and its exported objects. + // If not though, the consumer of the widget is expected to take ownership and close the widget appropriately. + let isCancelled = false; + async function loadWidgetInternal() { + try { + assertNotNull(fetch); + const newWidget = await fetch(); + if (isCancelled) { + log.debug2('loadWidgetInternal cancelled', descriptor, newWidget); + newWidget.close(); + if ('exportedObjects' in newWidget) { + newWidget.exportedObjects.forEach(exportedObject => { + exportedObject.close(); + }); + } + return; + } + log.debug('loadWidgetInternal done', descriptor, newWidget); + + setWrapper({ widget: newWidget, error: null }); + } catch (e) { + if (isCancelled) { + return; + } + log.error('loadWidgetInternal error', descriptor, e); + setWrapper({ widget: null, error: e ?? new Error('Null error') }); + } + } + loadWidgetInternal(); + return () => { + isCancelled = true; + }; + }, + [descriptor, objectFetch] + ); + + return wrapper; +} + +export default useWidget; diff --git a/packages/jsapi-bootstrap/tsconfig.json b/packages/jsapi-bootstrap/tsconfig.json index 398812c85f..7c57a9efe0 100644 --- a/packages/jsapi-bootstrap/tsconfig.json +++ b/packages/jsapi-bootstrap/tsconfig.json @@ -9,6 +9,7 @@ "references": [ { "path": "../components" }, { "path": "../log" }, - { "path": "../react-hooks" } + { "path": "../react-hooks" }, + { "path": "../utils" } ] }