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 5 commits
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
4 changes: 3 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

27 changes: 25 additions & 2 deletions packages/app-utils/src/components/ConnectionBootstrap.tsx
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -31,6 +33,7 @@ export function ConnectionBootstrap({
const client = useClient();
const [error, setError] = useState<unknown>();
const [connection, setConnection] = useState<dh.IdeConnection>();

useEffect(
function initConnection() {
let isCanceled = false;
Expand Down Expand Up @@ -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),
error: null,
});
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 (
<LoadingOverlay
Expand All @@ -96,7 +117,9 @@ export function ConnectionBootstrap({
return (
<ConnectionContext.Provider value={connection}>
<ObjectFetcherContext.Provider value={objectFetcher}>
{children}
<ObjectFetchManagerContext.Provider value={objectManager}>
{children}
</ObjectFetchManagerContext.Provider>
</ObjectFetcherContext.Provider>
</ConnectionContext.Provider>
);
Expand Down
3 changes: 2 additions & 1 deletion packages/jsapi-bootstrap/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
2 changes: 2 additions & 0 deletions packages/jsapi-bootstrap/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
35 changes: 35 additions & 0 deletions packages/jsapi-bootstrap/src/useObjectFetch.test.tsx
Original file line number Diff line number Diff line change
@@ -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, error: null });
return unsubscribe;
});
const objectManager = { subscribe };
const wrapper = ({ children }) => (
<ObjectFetchManagerContext.Provider value={objectManager}>
{children}
</ObjectFetchManagerContext.Provider>
);

const { result } = renderHook(() => useObjectFetch(descriptor), { wrapper });
expect(result.current).toEqual({ fetch: objectFetch, error: null });
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({
fetch: null,
error: expect.any(Error),
});
expect(result.error).toBeUndefined();
});
93 changes: 93 additions & 0 deletions packages/jsapi-bootstrap/src/useObjectFetch.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
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 with the current `fetch` fuonction and status of the object.
mofojed marked this conversation as resolved.
Show resolved Hide resolved
* - 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> = {
mofojed marked this conversation as resolved.
Show resolved Hide resolved
/**
* 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;
mofojed marked this conversation as resolved.
Show resolved Hide resolved
};

export type ObjectFetchUpdateCallback<T = unknown> = (
update: ObjectFetchUpdate<T>
) => void;

/** ObjectFetchManager for managing a subscription to an object using a VariableDescriptor */
export type ObjectFetchManager = {
/**
*
* @param descriptor Descriptor object to fetch the object from. Can be extended by a specific implementation to include more details necessary for the ObjectManager.
mofojed marked this conversation as resolved.
Show resolved Hide resolved
* @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.
*/
subscribe: <T = unknown>(
descriptor: dh.ide.VariableDescriptor,
onUpdate: ObjectFetchUpdateCallback<T>
) => UnsubscribeFunction;
};

/** Context for tracking an implementation of the ObjectFetchManager. */
export const ObjectFetchManagerContext =
createContext<ObjectFetchManager | null>(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<T = unknown>(
descriptor: dh.ide.VariableDescriptor
): ObjectFetchUpdate<T> {
const [currentUpdate, setCurrentUpdate] = useState<ObjectFetchUpdate<T>>(
() => ({
fetch: null,
error: null,
})
);
mofojed marked this conversation as resolved.
Show resolved Hide resolved

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,
});
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to be able to fetch without the object manager?

The error in the first block says no manager available, but that's true in both blocks

Copy link
Collaborator

Choose a reason for hiding this comment

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

This also makes me wonder if these 2 should be part of the same context. Is there a case where someone should be getting just the object fetcher and not using the object fetch manager?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm yea they could be combined... ObjectFetcher would be if you just want to fetch an object once, not subscribe to it and listen for updates... though not sure when you actually would want to do that.

return;
}
// Signal that we're still loading
setCurrentUpdate({
fetch: null,
error: null,
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this necessary since the state is already set to this initially? I guess it would be if objectFetchManager changed

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, I guess technically I could check if the old update is still fetch: null; error: null... it would technically matter if you are just looking at the object instead of destructuring it right away, as it's a new object...
I guess there's no real reason to fallback to the objectFetcher either. I should just take that out.

return objectFetchManager.subscribe(descriptor, setCurrentUpdate);
}, [descriptor, objectFetcher, objectFetchManager]);

return currentUpdate;
}
68 changes: 68 additions & 0 deletions packages/jsapi-bootstrap/src/useWidget.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import React from 'react';
import { act, renderHook } from '@testing-library/react-hooks';
import { TestUtils } from '@deephaven/utils';
import { useWidget } from './useWidget';
import { ObjectFetchManagerContext } from './useObjectFetch';

describe('useWidget', () => {
it('should return a widget when available', async () => {
const descriptor = { type: 'type', 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 }) => (
<ObjectFetchManagerContext.Provider value={objectManager}>
{children}
</ObjectFetchManagerContext.Provider>
);
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 = { type: 'type', name: 'name' };
const error = new Error('Error fetching widget');
const objectFetch = { fetch: null, 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>
);

const { result } = renderHook(() => useWidget(descriptor), { wrapper });

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

it('should return null when still loading', () => {
const descriptor = { type: 'type', name: 'name' };
const objectFetch = { fetch: null, error: null };
const subscribe = jest.fn((_, onUpdate) => {
onUpdate(objectFetch);
return jest.fn();
});
const objectManager = { subscribe };
const wrapper = ({ children }) => (
<ObjectFetchManagerContext.Provider value={objectManager}>
{children}
</ObjectFetchManagerContext.Provider>
);
const { result } = renderHook(() => useWidget(descriptor), { wrapper });

expect(result.current).toEqual({ widget: null, error: null });
});
});
93 changes: 93 additions & 0 deletions packages/jsapi-bootstrap/src/useWidget.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
import { 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');

/**
* Wrapper object for a widget and error status. Both widget and error will be `null` if it is still loading.
*/
type WidgetWrapper<T extends dh.Widget = dh.Widget> = {
/** Widget object to retrieve */
widget: T | null;

/** Error status if there was an issue fetching the widget */
error: unknown | null;
mofojed marked this conversation as resolved.
Show resolved Hide resolved
};

/**
* 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
mofojed marked this conversation as resolved.
Show resolved Hide resolved
* @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>(
descriptor: dh.ide.VariableDescriptor
): WidgetWrapper<T> {
const [wrapper, setWrapper] = useState<WidgetWrapper<T>>(() => ({
widget: null,
error: null,
}));

const objectFetch = useObjectFetch<T>(descriptor);

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

const { fetch, error } = objectFetch;

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

if (fetch == null) {
// Still loading
setWrapper({ widget: null, error: null });
return;
}

// 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();
newWidget.exportedObjects.forEach(
mofojed marked this conversation as resolved.
Show resolved Hide resolved
(exportedObject: dh.WidgetExportedObject) => {
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 });
}
}
loadWidgetInternal();
return () => {
isCancelled = true;
};
},
[descriptor, objectFetch]
);

return wrapper;
}

export default useWidget;
3 changes: 2 additions & 1 deletion packages/jsapi-bootstrap/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
"references": [
{ "path": "../components" },
{ "path": "../log" },
{ "path": "../react-hooks" }
{ "path": "../react-hooks" },
{ "path": "../utils" }
]
}
Loading