Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: DH-16737 Add ObjectManager, useWidget hook #2030

Merged
merged 8 commits into from
Jun 4, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/app-utils/src/components/ConnectionBootstrap.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ export function ConnectionBootstrap({
// We send an update with the fetch right away
onUpdate({
fetch: () => objectFetcher(descriptor),
error: null,
status: 'ready',
mofojed marked this conversation as resolved.
Show resolved Hide resolved
});
return () => {
// no-op
Expand Down
6 changes: 3 additions & 3 deletions packages/jsapi-bootstrap/src/useObjectFetch.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ it('should resolve the objectFetch when in the context', async () => {
const descriptor = { type: 'type', name: 'name' };
const subscribe = jest.fn((subscribeDescriptor, onUpdate) => {
expect(subscribeDescriptor).toEqual(descriptor);
onUpdate({ fetch: objectFetch, error: null });
onUpdate({ fetch: objectFetch, status: 'ready' });
return unsubscribe;
});
const objectManager = { subscribe };
Expand All @@ -19,7 +19,7 @@ it('should resolve the objectFetch when in the context', async () => {
);

const { result } = renderHook(() => useObjectFetch(descriptor), { wrapper });
expect(result.current).toEqual({ fetch: objectFetch, error: null });
expect(result.current).toEqual({ fetch: objectFetch, status: 'ready' });
expect(result.error).toBeUndefined();
expect(objectFetch).not.toHaveBeenCalled();
});
Expand All @@ -28,8 +28,8 @@ it('should return an error, not throw if objectFetch not available in the contex
const descriptor = { type: 'type', name: 'name' };
const { result } = renderHook(() => useObjectFetch(descriptor));
expect(result.current).toEqual({
fetch: null,
error: expect.any(Error),
status: 'error',
});
expect(result.error).toBeUndefined();
});
74 changes: 35 additions & 39 deletions packages/jsapi-bootstrap/src/useObjectFetch.ts
Original file line number Diff line number Diff line change
@@ -1,28 +1,36 @@
import { createContext, useContext, useEffect, useState } from 'react';
import type { dh } from '@deephaven/jsapi-types';
import { ObjectFetcherContext } from './useObjectFetcher';

/** 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<unknown>;
status: 'error';
};

/** Update when the object is ready */
export type ObjectFetchReady<T> = {
fetch: () => Promise<T>;
status: 'ready';
};

/**
* Update with the current `fetch` fuonction and status of the object.
* 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<T = unknown> = {
/**
* Function to fetch the object. If `null`, the object is still loading or there was an error.
*/
fetch: (() => Promise<T>) | null;

/**
* Error that occurred while fetching the object. If `null`, there was no error.
* Will automatically retry when possible while the subscribed.
*/
error: unknown | null;
};
export type ObjectFetchUpdate<T = unknown> =
| ObjectFetchLoading
| ObjectFetchError
| ObjectFetchReady<T>;

export type ObjectFetchUpdateCallback<T = unknown> = (
update: ObjectFetchUpdate<T>
Expand All @@ -31,7 +39,7 @@ export type ObjectFetchUpdateCallback<T = unknown> = (
/** ObjectFetchManager for managing a subscription to an object using a VariableDescriptor */
export type ObjectFetchManager = {
/**
*
* Subscribe to an object using a variable descriptor.
mofojed marked this conversation as resolved.
Show resolved Hide resolved
* @param descriptor Descriptor object to fetch the object from. 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 updates and clean up the object.
Expand All @@ -56,38 +64,26 @@ export const ObjectFetchManagerContext =
export function useObjectFetch<T = unknown>(
descriptor: dh.ide.VariableDescriptor
): ObjectFetchUpdate<T> {
const [currentUpdate, setCurrentUpdate] = useState<ObjectFetchUpdate<T>>(
() => ({
fetch: null,
error: null,
})
);
const [currentUpdate, setCurrentUpdate] = useState<ObjectFetchUpdate<T>>({
status: 'loading',
});

const objectFetcher = useContext(ObjectFetcherContext);
const objectFetchManager = useContext(ObjectFetchManagerContext);

useEffect(() => {
if (objectFetchManager == null) {
if (objectFetcher == null) {
setCurrentUpdate({
fetch: null,
error: new Error('No ObjectFetchManager available in context'),
});
} else {
setCurrentUpdate({
fetch: () => objectFetcher(descriptor),
error: null,
});
}
setCurrentUpdate({
error: new Error('No ObjectFetchManager available in context'),
status: 'error',
});
return;
}
// Signal that we're still loading
setCurrentUpdate({
fetch: null,
error: null,
});
// 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, objectFetcher, objectFetchManager]);
}, [descriptor, objectFetchManager]);

return currentUpdate;
}
67 changes: 53 additions & 14 deletions packages/jsapi-bootstrap/src/useWidget.test.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,26 @@
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';
import { ApiContext } from './ApiBootstrap';

const WIDGET_TYPE = 'OtherWidget';

const api = TestUtils.createMockProxy<typeof dh>({
VariableType: TestUtils.createMockProxy<typeof dh.VariableType>({
OTHERWIDGET: WIDGET_TYPE,
TABLE: 'Table',
}),
});

describe('useWidget', () => {
it('should return a widget when available', async () => {
const descriptor = { type: 'type', name: 'name' };
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 };
Expand All @@ -17,9 +31,11 @@ describe('useWidget', () => {
});
const objectManager = { subscribe };
const wrapper = ({ children }) => (
<ObjectFetchManagerContext.Provider value={objectManager}>
{children}
</ObjectFetchManagerContext.Provider>
<ApiContext.Provider value={api}>
<ObjectFetchManagerContext.Provider value={objectManager}>
{children}
</ObjectFetchManagerContext.Provider>
</ApiContext.Provider>
);
const { result } = renderHook(() => useWidget(descriptor), { wrapper });
await act(TestUtils.flushPromises);
Expand All @@ -28,19 +44,24 @@ describe('useWidget', () => {
});

it('should return an error when an error occurs', () => {
const descriptor = { type: 'type', name: 'name' };
const descriptor: dh.ide.VariableDescriptor = {
type: WIDGET_TYPE,
name: 'name',
};
const error = new Error('Error fetching widget');
const objectFetch = { fetch: null, error };
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 }) => (
<ObjectFetchManagerContext.Provider value={objectManager}>
{children}
</ObjectFetchManagerContext.Provider>
<ApiContext.Provider value={api}>
<ObjectFetchManagerContext.Provider value={objectManager}>
{children}
</ObjectFetchManagerContext.Provider>
</ApiContext.Provider>
);

const { result } = renderHook(() => useWidget(descriptor), { wrapper });
Expand All @@ -49,20 +70,38 @@ describe('useWidget', () => {
});

it('should return null when still loading', () => {
const descriptor = { type: 'type', name: 'name' };
const objectFetch = { fetch: null, error: null };
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 }) => (
<ObjectFetchManagerContext.Provider value={objectManager}>
{children}
</ObjectFetchManagerContext.Provider>
<ApiContext.Provider value={api}>
<ObjectFetchManagerContext.Provider value={objectManager}>
{children}
</ObjectFetchManagerContext.Provider>
</ApiContext.Provider>
);
const { result } = renderHook(() => useWidget(descriptor), { wrapper });

expect(result.current).toEqual({ widget: null, error: null });
});

it('should return an error when the descriptor type is not supported', () => {
const descriptor: dh.ide.VariableDescriptor = {
type: 'Table',
name: 'name',
};
const wrapper = ({ children }) => (
<ApiContext.Provider value={api}>{children}</ApiContext.Provider>
);
const { result } = renderHook(() => useWidget(descriptor), { wrapper });

expect(result.current).toEqual({
widget: null,
error: new Error(`Unsupported descriptor type: ${descriptor.type}`),
});
});
});
44 changes: 34 additions & 10 deletions packages/jsapi-bootstrap/src/useWidget.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import { dh } from '@deephaven/jsapi-types';
import Log from '@deephaven/log';
import { assertNotNull } from '@deephaven/utils';
import { useEffect, useState } from 'react';
import { useEffect, useMemo, useState } from 'react';
import useApi from './useApi';
import { useObjectFetch } from './useObjectFetch';

const log = Log.module('useWidget');
Expand All @@ -14,12 +15,12 @@ type WidgetWrapper<T extends dh.Widget = dh.Widget> = {
widget: T | null;

/** Error status if there was an issue fetching the widget */
error: unknown | null;
error: NonNullable<unknown> | 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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there frequently cases where we wouldn't want to just close this when useWidget unmounts? Wondering if the norm should be we clean it up automatically and provide a config option to not clean it up automatically if you have a good reason.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, with a widget you can get updates that include new exported objects (like we do with deephaven.ui), those need to be cleaned up as well; or you may want to re-export exported objects rather than fetch them, I don't think we want to automatically close things here as it's not clear if the objects were actually fetched by the consumer or not.

* @param descriptor Descriptor to get the widget for
* @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<T extends dh.Widget = dh.Widget>(
Expand All @@ -29,27 +30,50 @@ export function useWidget<T extends dh.Widget = dh.Widget>(
widget: null,
error: null,
}));

const api = useApi();
const unsupportedTypes = useMemo(
() => [
api.VariableType.TABLE,
api.VariableType.TREETABLE,
api.VariableType.HIERARCHICALTABLE,
api.VariableType.TABLEMAP,
api.VariableType.PARTITIONEDTABLE,
api.VariableType.FIGURE,
api.VariableType.PANDAS,
api.VariableType.TREEMAP,
],
mofojed marked this conversation as resolved.
Show resolved Hide resolved
[api]
);
const objectFetch = useObjectFetch<T>(descriptor);

useEffect(
function loadWidget() {
log.debug('loadWidget', descriptor);

const { fetch, error } = objectFetch;
if (unsupportedTypes.includes(descriptor.type)) {
// We only support fetching widgets with this hook
setWrapper({
widget: null,
error: new Error(`Unsupported descriptor type: ${descriptor.type}`),
});
return;
}

const { status } = objectFetch;

if (error != null) {
if (status === 'error') {
// We can't fetch if there's an error getting the fetcher, just return an error
setWrapper({ widget: null, error });
setWrapper({ widget: null, error: objectFetch.error });
return;
}

if (fetch == null) {
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.
Expand All @@ -76,15 +100,15 @@ export function useWidget<T extends dh.Widget = dh.Widget>(
return;
}
log.error('loadWidgetInternal error', descriptor, e);
setWrapper({ widget: null, error: e });
setWrapper({ widget: null, error: e ?? new Error('Null error') });
}
}
loadWidgetInternal();
return () => {
isCancelled = true;
};
},
[descriptor, objectFetch]
[descriptor, objectFetch, unsupportedTypes]
);

return wrapper;
Expand Down
Loading