Skip to content

Commit

Permalink
Changed parseData method to parseDocument
Browse files Browse the repository at this point in the history 
- It cleans up dead objects, which we only want to do when parsing an actual documentUpdated request rather than decoding any response (such as a response for a callback)
- Cleaned up how ElementMessageStream encodes the payload and sends it back.
  • Loading branch information
@mofojed
mofojed committed Dec 4, 2023
1 parent 3f529d2 commit e3464d7
Show file tree
Hide file tree
Showing 4 changed files with 53 additions and 100 deletions.
13 changes: 7 additions & 6 deletions plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from __future__ import annotations
import io
import json
from jsonrpc import JSONRPCResponseManager, Dispatcher
import logging
from typing import Any
Expand Down Expand Up @@ -83,9 +84,9 @@ def on_data(self, payload: bytes, references: list[Any]) -> None:
if response is None:
return

payload = response.json
logger.debug("Response: %s, %s", type(payload), payload)
self._connection.on_data(payload.encode(), [])
response_payload = response.json
logger.debug("Response: %s, %s", type(response_payload), response_payload)
self._connection.on_data(response_payload.encode(), [])

def _get_next_message_id(self) -> int:
self._message_id += 1
Expand Down Expand Up @@ -129,9 +130,9 @@ def _send_document_update(self, root: RenderedNode) -> None:
"""

# TODO(#67): Send a diff of the document instead of the entire document.
request = self._make_notification("documentUpdated", root)
payload = self._encoder.encode(request)

encoded_document = self._encoder.encode(root)
request = self._make_notification("documentUpdated", encoded_document)
payload = json.dumps(request)
logger.debug(f"Sending payload: {payload}")

dispatcher = Dispatcher()
Expand Down
28 changes: 6 additions & 22 deletions plugins/ui/src/js/src/UITable.tsx
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,5 @@
import React, { useEffect, useMemo, useState } from 'react';
import {
IrisGridModel,
IrisGridModelFactory,
IrisGridProps,
} from '@deephaven/iris-grid';
import { IrisGridProps } from '@deephaven/iris-grid';
import { useApi } from '@deephaven/jsapi-bootstrap';
import type { Table } from '@deephaven/jsapi-types';
import Log from '@deephaven/log';
Expand All @@ -18,36 +14,24 @@ export interface UITableProps {

function UITable({ element }: UITableProps) {
const dh = useApi();
const [model, setModel] = useState<IrisGridModel>();
const [table, setTable] = useState<Table>();
const { props: elementProps } = element;

// Just load the object on mount
useEffect(() => {
let isCancelled = false;
async function loadModel() {
log.debug('Loading table from props', elementProps);
const newTable = (await elementProps.table.fetch()) as Table;
const newModel = await IrisGridModelFactory.makeModel(dh, newTable);
if (!isCancelled) {
setModel(newModel);
} else {
newModel.close();
}
log.debug('Loading table from props', element.props);
const newTable = (await element.props.table.fetch()) as Table;
setTable(newTable);
}
loadModel();
return () => {
isCancelled = true;
};
}, [dh, elementProps]);
}, [dh, element]);

const irisGridProps: Partial<IrisGridProps> = useMemo(() => {
const { onRowDoublePress } = elementProps;
return { onDataSelected: onRowDoublePress };
}, [elementProps]);

// We want to clean up the model when we unmount or get a new model
useEffect(() => () => model?.close(), [model]);

return table ? (
<TableObject object={table} irisGridProps={irisGridProps} />
) : null;
Expand Down
110 changes: 39 additions & 71 deletions plugins/ui/src/js/src/WidgetHandler.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,17 +50,6 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) {
);
const exportedObjectCount = useRef(0);

useEffect(
() =>
function closeWidget() {
exportedObjectMap.current.forEach(exportedObject => {
exportedObject.close();
});
widget?.close();
},
[widget]
);

// Bi-directional communication as defined in https://www.npmjs.com/package/json-rpc-2.0
const jsonClient = useMemo(
() =>
Expand All @@ -75,69 +64,20 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) {
: null,
[widget]
);
const parseData = useCallback(
const parseDocument = useCallback(
/**
* Parse the data from the server, replacing any callable nodes with functions that call the server.
* Replaces all Callables with an async callback that will automatically call the server use JSON-RPC.
* Replaces all Objects with the exported object from the server.
* Element nodes are not replaced. Those are handled in `DocumentHandler`.
*
* @param data The data to parse
* @param newExportedObjects New exported objects to add to the map
* @returns The parsed data
*/
(data: string, newExportedObjects: WidgetExportedObject[]) => {
(data: string) => {
// Keep track of exported objects that are no longer in use after this render.
// We close those objects that are no longer referenced, as they will never be referenced again.
const deadObjectMap = new Map(exportedObjectMap.current);
const newExportedObjectMap = new Map(exportedObjectMap.current);

for (let i = 0; i < newExportedObjects.length; i += 1) {
const exportedObject = newExportedObjects[i];
const exportedObjectKey = exportedObjectCount.current;
exportedObjectCount.current += 1;

// if (exportedObject.type === 'Table') {
// // Table has some special handling compared to other widgets
// // We want to return a copy of the table, and only release the table object when the widget is actually closed
// }
// Some elements may fetch the object, then be hidden and close the object temporarily, and then shown again.
// We can only fetch each exported object once, so just fetch it once and cache it, then subscribe/unsubscribe as needed.
const cachedObject: [Promise<unknown> | undefined] = [undefined];
const proxyObject = new Proxy(exportedObject, {
get: (target, prop, ...rest) => {
if (prop === 'fetch') {
return async () => {
const newObject = await target.fetch();
console.log('XXX newObj', newObject);
const newObj2 = await target.fetch();
console.log('XXX newObj2', newObj2);
(newObject as any).close();
console.log('XXX newObj closed');
const newObj3 = await target.fetch();
console.log('XXX newObj3', newObj3);
(newObj2 as any).close();
return newObj3;
};
// return () => {
// if (cachedObject[0] === undefined) {
// cachedObject[0] = target.fetch();
// }
// return cachedObject[0];
// };
}
// if (prop === 'close') {
// return () => {
// // We only want to unsubscribe from the object here, not close it. We will close it when the widget is closed, but until then the object may be needed again.
// (cachedObject[0] as any).unsubscribe();
// };
// }
return Reflect.get(target, prop, ...rest);
},
});

newExportedObjectMap.set(exportedObjectKey, proxyObject);
}

const parsedData = JSON.parse(data, (key, value) => {
// Need to re-hydrate any objects that are defined
Expand All @@ -152,7 +92,7 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) {
if (isObjectNode(value)) {
// Replace this node with the exported object
const objectKey = value[OBJECT_KEY];
const exportedObject = newExportedObjectMap.get(objectKey);
const exportedObject = exportedObjectMap.current.get(objectKey);
if (exportedObject === undefined) {
// The map should always have the exported object for a key, otherwise the protocol is broken
throw new Error(`Invalid exported object key ${objectKey}`);
Expand All @@ -168,32 +108,52 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) {
deadObjectMap.forEach((deadObject, objectKey) => {
log.debug('Closing dead object', objectKey);
deadObject.close();
newExportedObjectMap.delete(objectKey);
exportedObjectMap.current.delete(objectKey);
});

exportedObjectMap.current = newExportedObjectMap;
log.debug2(
'Parsed data',
parsedData,
'exportedObjectMap',
exportedObjectMap.current,
'deadObjectMap',
deadObjectMap
);
return parsedData;
},
[jsonClient]
);

const updateExportedObjects = useCallback(
(newExportedObjects: WidgetExportedObject[]) => {
for (let i = 0; i < newExportedObjects.length; i += 1) {
const exportedObject = newExportedObjects[i];
const exportedObjectKey = exportedObjectCount.current;
exportedObjectCount.current += 1;
exportedObjectMap.current.set(exportedObjectKey, exportedObject);
}
},
[]
);

useEffect(
function initMethods() {
if (jsonClient == null) {
return;
}

log.debug('Adding methods to jsonClient');
jsonClient.addMethod('documentUpdated', async (params: [ElementNode]) => {
jsonClient.addMethod('documentUpdated', async (params: [string]) => {
log.debug2('documentUpdated', params[0]);
setElement(params[0]);
const newDocument = parseDocument(params[0]);
setElement(newDocument);
});

return () => {
jsonClient.rejectAllPendingRequests('Widget was changed');
};
},
[jsonClient]
[jsonClient, parseDocument]
);

useEffect(() => {
Expand All @@ -205,8 +165,8 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) {
newExportedObjects: WidgetExportedObject[]
) {
log.debug2('Data received', data, newExportedObjects);
const parsedData = parseData(data, newExportedObjects);
jsonClient?.receiveAndSend(parsedData);
updateExportedObjects(newExportedObjects);
jsonClient?.receiveAndSend(JSON.parse(data));
}

const cleanup = widget.addEventListener(
Expand All @@ -227,7 +187,7 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) {
log.debug('Cleaning up listener');
cleanup();
};
}, [dh, jsonClient, parseData, widget]);
}, [dh, jsonClient, parseDocument, updateExportedObjects, widget]);

useEffect(
function loadWidget() {
Expand All @@ -250,6 +210,14 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) {
[wrapper]
);

useEffect(
() =>
function closeWidget() {
widget?.close();
},
[widget]
);

const handleDocumentClose = useCallback(() => {
log.debug('Widget document closed', wrapper.id);
onClose?.(wrapper.id);
Expand Down
2 changes: 1 addition & 1 deletion plugins/ui/src/js/src/WidgetTestUtils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ export function makeDocumentUpdatedJsonRpc(
return {
jsonrpc: '2.0',
method: 'documentUpdated',
params: [document],
params: [JSON.stringify(document)],
};
}

Expand Down

0 comments on commit e3464d7

Please sign in to comment.