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

fix: Reset state when new instance of widget created #486

Merged
merged 3 commits into from
May 24, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
239 changes: 229 additions & 10 deletions plugins/ui/src/js/src/widget/WidgetHandler.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,11 @@ import WidgetHandler, { WidgetHandlerProps } from './WidgetHandler';
import { DocumentHandlerProps } from './DocumentHandler';
import {
makeDocumentUpdatedJsonRpcString,
makeJsonRpcResponseString,
makeWidget,
makeWidgetDescriptor,
makeWidgetEventDocumentUpdated,
makeWidgetEventJsonRpcResponse,
} from './WidgetTestUtils';

const mockApi = { Widget: { EVENT_MESSAGE: 'message' } };
Expand All @@ -26,8 +29,16 @@ function makeWidgetHandler({
fetch = () => Promise.resolve(makeWidget()),
widget = makeWidgetDescriptor(),
onClose = jest.fn(),
initialData = undefined,
}: Partial<WidgetHandlerProps> = {}) {
return <WidgetHandler fetch={fetch} widget={widget} onClose={onClose} />;
return (
<WidgetHandler
fetch={fetch}
widget={widget}
onClose={onClose}
initialData={initialData}
/>
);
}

beforeEach(() => {
Expand All @@ -48,38 +59,77 @@ it('updates the document when event is received', async () => {
const widget = makeWidgetDescriptor();
const cleanup = jest.fn();
const mockAddEventListener = jest.fn(() => cleanup);
const mockSendMessage = jest.fn();
const initialData = { state: { fiz: 'baz' } };
const initialDocument = { foo: 'bar' };
const widgetObject = makeWidget({
addEventListener: mockAddEventListener,
getDataAsString: jest.fn(() =>
makeDocumentUpdatedJsonRpcString(initialDocument)
),
getDataAsString: jest.fn(() => ''),
sendMessage: mockSendMessage,
});

const { unmount } = render(makeWidgetHandler({ widget, fetch }));
const { unmount } = render(makeWidgetHandler({ widget, fetch, initialData }));
expect(fetch).toHaveBeenCalledTimes(1);
expect(mockAddEventListener).not.toHaveBeenCalled();
expect(mockDocumentHandler).not.toHaveBeenCalled();
expect(mockSendMessage).not.toHaveBeenCalled();
await act(async () => {
fetchResolve!(widgetObject);
await fetchPromise;
});

expect(mockAddEventListener).toHaveBeenCalledTimes(1);
expect(mockDocumentHandler).not.toHaveBeenCalled();

expect(mockSendMessage).toHaveBeenCalledWith(
JSON.stringify({
jsonrpc: '2.0',
id: 1,
method: 'setState',
params: [initialData.state],
}),
[]
);

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const listener = (mockAddEventListener.mock.calls[0] as any)[1];

// Send the initial document
await act(async () => {
// Respond to the setState call first
listener({
detail: {
getDataAsString: jest.fn(() => makeJsonRpcResponseString(1, {})),
exportedObjects: [],
},
});

// Then send the initial document update
listener({
detail: {
getDataAsString: jest.fn(() =>
makeDocumentUpdatedJsonRpcString(initialDocument)
),
exportedObjects: [],
},
});
});

expect(mockDocumentHandler).toHaveBeenCalledWith(
expect.objectContaining({
widget,
children: initialDocument,
initialData,
})
);

mockDocumentHandler.mockClear();
const updatedDocument = { FOO: 'BAR' };

const updatedDocument = { fiz: 'baz' };
mockDocumentHandler.mockClear();

act(() => {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(mockAddEventListener.mock.calls[0] as any)[1]({
// Send the updated document
await act(async () => {
listener({
detail: {
getDataAsString: jest.fn(() =>
makeDocumentUpdatedJsonRpcString(updatedDocument)
Expand All @@ -99,3 +149,172 @@ it('updates the document when event is received', async () => {
unmount();
expect(cleanup).toHaveBeenCalledTimes(1);
});

it('updates the initial data only when fetch has changed', async () => {
let fetchResolve1: (value: dh.Widget | PromiseLike<dh.Widget>) => void;
mofojed marked this conversation as resolved.
Show resolved Hide resolved
const fetchPromise1 = new Promise<dh.Widget>(resolve => {
fetchResolve1 = resolve;
});
const fetch1 = jest.fn(() => fetchPromise1);
const widget1 = makeWidgetDescriptor();
const cleanup = jest.fn();
const addEventListener = jest.fn(() => cleanup);
const sendMessage = jest.fn();
const onClose = jest.fn();
const data1 = { state: { fiz: 'baz' } };
const document1 = { foo: 'bar' };
const widgetObject1 = makeWidget({
addEventListener,
getDataAsString: jest.fn(() => ''),
sendMessage,
});

const { rerender, unmount } = render(
makeWidgetHandler({
widget: widget1,
fetch: fetch1,
initialData: data1,
onClose,
})
);
expect(fetch1).toHaveBeenCalledTimes(1);
expect(addEventListener).not.toHaveBeenCalled();
expect(mockDocumentHandler).not.toHaveBeenCalled();
expect(sendMessage).not.toHaveBeenCalled();
await act(async () => {
fetchResolve1!(widgetObject1);
await fetchPromise1;
});

expect(addEventListener).toHaveBeenCalledTimes(1);
expect(mockDocumentHandler).not.toHaveBeenCalled();

expect(sendMessage).toHaveBeenCalledWith(
JSON.stringify({
jsonrpc: '2.0',
id: 1,
method: 'setState',
params: [data1.state],
}),
[]
);

// eslint-disable-next-line @typescript-eslint/no-explicit-any
let listener = (addEventListener.mock.calls[0] as any)[1];

// Send the initial document
await act(async () => {
// Respond to the setState call first
listener(makeWidgetEventJsonRpcResponse(1));

// Then send the initial document update
listener(makeWidgetEventDocumentUpdated(document1));
});

expect(mockDocumentHandler).toHaveBeenCalledWith(
expect.objectContaining({
widget: widget1,
children: document1,
initialData: data1,
})
);

let fetchResolve2: (value: dh.Widget | PromiseLike<dh.Widget>) => void;
const fetchPromise2 = new Promise<dh.Widget>(resolve => {
fetchResolve2 = resolve;
});
const widget2 = makeWidgetDescriptor();
const document2 = { FOO: 'BAR' };
const data2 = { state: { FIZ: 'BAZ' } };
const fetch2 = jest.fn(() => fetchPromise2);
const widgetObject2 = makeWidget({
addEventListener,
getDataAsString: jest.fn(() => ''),
sendMessage,
});

addEventListener.mockClear();
mockDocumentHandler.mockClear();
sendMessage.mockClear();
fetch1.mockClear();

// Re-render with just initial data change. It should not set the state again
rerender(
makeWidgetHandler({
widget: widget1,
fetch: fetch1,
initialData: data2,
onClose,
})
);

expect(fetch1).not.toHaveBeenCalled();
expect(sendMessage).not.toHaveBeenCalled();

// Re-render with the widget descriptor changed, it should set the state with the updated data
rerender(
makeWidgetHandler({
widget: widget2,
fetch: fetch2,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the change to fetch what you want to test here or just the change to widget?

Copy link
Member Author

@mofojed mofojed May 24, 2024

Choose a reason for hiding this comment

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

The descriptor. I'll have another PR up shortly that will negate the need to pass in fetch entirely.

initialData: data2,
onClose,
})
);

await act(async () => {
fetchResolve2!(widgetObject2);
await fetchPromise2;
});

expect(fetch2).toHaveBeenCalledTimes(1);
// Should have been called when the widget was updated
expect(cleanup).toHaveBeenCalledTimes(1);
cleanup.mockClear();

// eslint-disable-next-line prefer-destructuring, @typescript-eslint/no-explicit-any
listener = (addEventListener.mock.calls[0] as any)[1];

expect(sendMessage).toHaveBeenCalledWith(
JSON.stringify({
jsonrpc: '2.0',
id: 1,
method: 'setState',
params: [data2.state],
}),
[]
);
expect(sendMessage).toHaveBeenCalledTimes(1);

// Send the initial document
await act(async () => {
// Respond to the setState call first
listener({
mofojed marked this conversation as resolved.
Show resolved Hide resolved
detail: {
getDataAsString: jest.fn(() => makeJsonRpcResponseString(1, {})),
exportedObjects: [],
},
});

// Then send the initial document update
listener({
detail: {
getDataAsString: jest.fn(() =>
makeDocumentUpdatedJsonRpcString(document2)
),
exportedObjects: [],
},
});
});

expect(mockDocumentHandler).toHaveBeenCalledWith(
expect.objectContaining({
widget: widget1,
children: document2,
initialData: data2,
})
);

expect(cleanup).not.toHaveBeenCalled();
unmount();
expect(cleanup).toHaveBeenCalledTimes(1);
});
5 changes: 4 additions & 1 deletion plugins/ui/src/js/src/widget/WidgetHandler.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,10 @@ function WidgetHandler({
const [widget, setWidget] = useState<dh.Widget>();
const [document, setDocument] = useState<ReactNode>();
const [error, setError] = useState<WidgetError>();
const [initialData] = useState(initialDataProp);

// We want to update the initial data if the widget changes, as we'll need to re-fetch the widget and want to start with a fresh state.
// eslint-disable-next-line react-hooks/exhaustive-deps
const initialData = useMemo(() => initialDataProp, [widget]);

// When we fetch a widget, the client is then responsible for the exported objects.
// These objects could stay alive even after the widget is closed if we wanted to,
Expand Down
34 changes: 34 additions & 0 deletions plugins/ui/src/js/src/widget/WidgetTestUtils.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { WidgetDescriptor } from '@deephaven/dashboard';
import { TestUtils } from '@deephaven/utils';
import type { dh } from '@deephaven/jsapi-types';
import { WidgetMessageEvent } from './WidgetTypes';

export function makeDocumentUpdatedJsonRpc(
document: Record<string, unknown> = {}
Expand All @@ -12,12 +13,43 @@ export function makeDocumentUpdatedJsonRpc(
};
}

export function makeJsonRpcResponseString(id: number, result = ''): string {
return JSON.stringify({
jsonrpc: '2.0',
id,
result,
});
}

export function makeDocumentUpdatedJsonRpcString(
document: Record<string, unknown> = {}
): string {
return JSON.stringify(makeDocumentUpdatedJsonRpc(document));
}

export function makeWidgetEvent(data = ''): WidgetMessageEvent {
return new CustomEvent('message', {
detail: {
getDataAsBase64: () => '',
getDataAsString: () => data,
exportedObjects: [],
},
});
}

export function makeWidgetEventJsonRpcResponse(
id: number,
response = ''
): WidgetMessageEvent {
return makeWidgetEvent(makeJsonRpcResponseString(id, response));
}

export function makeWidgetEventDocumentUpdated(
document: Record<string, unknown> = {}
): WidgetMessageEvent {
return makeWidgetEvent(makeDocumentUpdatedJsonRpcString(document));
}

export function makeWidgetDescriptor({
id = 'widget-id',
type = 'widget-type',
Expand All @@ -34,10 +66,12 @@ export function makeWidget({
addEventListener = jest.fn(() => jest.fn()),
getDataAsString = () => makeDocumentUpdatedJsonRpcString(),
exportedObjects = [],
sendMessage = jest.fn(),
}: Partial<dh.Widget> = {}): dh.Widget {
return TestUtils.createMockProxy<dh.Widget>({
addEventListener,
getDataAsString,
exportedObjects,
sendMessage,
});
}
Loading