Skip to content

Commit

Permalink
Cleanup based on review
Browse files Browse the repository at this point in the history
- Use useDeferredApi instead of fetching from context directly
- Allow null to be passed in for descriptor in useDeferredApi and throw an error accordingly
  • Loading branch information
mofojed committed Aug 20, 2024
1 parent f1d3040 commit 5c78f1d
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 27 deletions.
31 changes: 8 additions & 23 deletions packages/dashboard-core-plugins/src/ChartPanelPlugin.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { forwardRef, useContext, useMemo } from 'react';
import { DeferredApiContext } from '@deephaven/jsapi-bootstrap';
import { forwardRef, useMemo } from 'react';
import { useDeferredApi } from '@deephaven/jsapi-bootstrap';
import { ChartModel, ChartModelFactory } from '@deephaven/chart';
import type { dh as DhType } from '@deephaven/jsapi-types';
import { IrisGridUtils } from '@deephaven/iris-grid';
Expand All @@ -23,18 +23,14 @@ async function createChartModel(
panelFetch: () => Promise<DhType.plot.Figure | DhType.Table>,
panelState?: GLChartPanelState
): Promise<ChartModel> {
let settings;
let tableName;
let tableSettings;
let settings = {};
let tableName = '';
let tableSettings = {};

if (isChartPanelTableMetadata(metadata)) {
settings = metadata.settings;
tableName = metadata.table;
tableSettings = metadata.tableSettings;
} else {
settings = {};
tableName = '';
tableSettings = {};
}
if (panelState != null) {
if (panelState.tableSettings != null) {
Expand Down Expand Up @@ -69,30 +65,19 @@ async function createChartModel(

export const ChartPanelPlugin = forwardRef(
(props: WidgetPanelProps<DhType.plot.Figure>, ref: React.Ref<ChartPanel>) => {
const deferredApi = useContext(DeferredApiContext);

const panelState = isChartPanelDehydratedProps(props)
? (props as unknown as ChartPanelProps).panelState
: undefined;

const { fetch: panelFetch, metadata, localDashboardId } = props;
const [dh, error] = useDeferredApi(metadata ?? null);

const hydratedProps = useMemo(
() => ({
metadata: metadata as ChartPanelMetadata,
localDashboardId,
makeModel: async () => {
if (metadata == null) {
throw new Error('Metadata is required for chart panel');
}

const dh =
typeof deferredApi === 'function'
? await deferredApi(metadata)
: deferredApi;

assertNotNull(dh, `Cannot find API for metadata: ${metadata}`);

assertNotNull(dh, `Cannot find API: ${error}`);
return createChartModel(
dh,
metadata as ChartPanelMetadata,
Expand All @@ -101,7 +86,7 @@ export const ChartPanelPlugin = forwardRef(
);
},
}),
[metadata, localDashboardId, deferredApi, panelFetch, panelState]
[dh, error, metadata, localDashboardId, panelFetch, panelState]
);

// eslint-disable-next-line react/jsx-props-no-spreading
Expand Down
11 changes: 10 additions & 1 deletion packages/jsapi-bootstrap/src/useDeferredApi.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ describe('useDeferredApi', () => {
expect(result.current).toEqual([dh1, null]);

const { result: result2 } = renderHook(() =>
useDeferredApi({ type: 'foo', foo: 'bar' })
useDeferredApi({
type: 'foo',
foo: 'bar',
} as DhType.ide.VariableDescriptor)
);
expect(result2.current).toEqual([dh1, null]);
});
Expand Down Expand Up @@ -68,4 +71,10 @@ describe('useDeferredApi', () => {
const { result } = renderHook(() => useDeferredApi(objectMetadata));
expect(result.current).toEqual([null, expect.any(Error)]);
});

it('returns an error if the metadata is null', async () => {
asMock(useContext).mockReturnValue(dh1);
const { result } = renderHook(() => useDeferredApi(null));
expect(result.current).toEqual([null, expect.any(Error)]);
});
});
11 changes: 8 additions & 3 deletions packages/jsapi-bootstrap/src/useDeferredApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ export const DeferredApiContext = createContext<
* @returns A tuple with the API instance, and an error if one occurred.
*/
export function useDeferredApi(
widget: DhType.ide.VariableDescriptor
): [typeof DhType | null, unknown | null] {
widget: DhType.ide.VariableDescriptor | null
): [dh: typeof DhType | null, error: unknown | null] {
const [api, setApi] = useState<typeof DhType | null>(null);
const [error, setError] = useState<unknown | null>(null);
const deferredApi = useContext(DeferredApiContext);
Expand All @@ -49,7 +49,12 @@ export function useDeferredApi(
let isCancelled = false;

async function loadApi() {
if (typeof deferredApi === 'function') {
if (widget == null) {
if (!isCancelled) {
setApi(null);
setError(new Error('No widget provided to useDeferredApi'));
}
} else if (typeof deferredApi === 'function') {
try {
const newApi = await deferredApi(widget);
if (!isCancelled) {
Expand Down

0 comments on commit 5c78f1d

Please sign in to comment.