feat: JS API support for shared tickets #5854
Conversation
niloc132
added
jsapi
NoDocumentationNeeded
ReleaseNotesNeeded
web/client-api/src/main/java/io/deephaven/web/client/ide/IdeSession.java
Outdated
Show resolved
Hide resolved
web/client-api/src/main/java/io/deephaven/web/client/ide/IdeConnection.java
Outdated
Show resolved
Hide resolved
| @@ -118,6 +118,11 @@ public JsWidget(WorkerConnection connection, TypedTicket typedTicket) { | |||
| this.exportedObjects = new JsArray<>(); | |||
| } | |||
|
|
|||
| @Override | |||
There was a problem hiding this comment.
Unnecessary, this isn't a @JsType, so public members arent exported by default.
| @@ -62,6 +62,11 @@ public JsWidgetExportedObject(WorkerConnection connection, TypedTicket ticket) { | |||
| }); | |||
| } | |||
|
|
|||
| @Override | |||
There was a problem hiding this comment.
Same as above, we don't export the whole type, so it doesnt need to be ignored.
| bytesWithPrefix.setAt(0, (double) 'h'); | ||
| bytesWithPrefix.setAt(1, (double) '/'); |
There was a problem hiding this comment.
- What is this magic
h/, what does it mean? Comment would be useful here.
There was a problem hiding this comment.
Every ticket type has a one char prefix followed by a slash - s is for scope tickets, e is for export tickets. The h prefix is what the server documents for shared tickets.
There was a problem hiding this comment.
Is there anywhere all the prefixes are documented?
There was a problem hiding this comment.
No - scope handling is technically optional aside from export tickets, downstream server implementations can skip various ticket resolvers, or add their own.
There was a problem hiding this comment.
Okay, so it's optional? But we're relying on it here?
Can we please add a comment in the code detailing this magic and possibly link to our optional implementation code that is adding these scopes?
There was a problem hiding this comment.
This is non-optional - all tickets being sent to a DH server must have a prefix of this form. Shared tickets never must supply this (in fact there is a test explicitly to confirm that h/foo is not the same as foo, just in case of this kind of confusion) unless it is part of their own naming scheme.
There's no magic here, this is always required for export/scope/application tickets, and for any other custom ticket resolver that any downstream implementation adds (see DHE's database tickets, for example). Application/scope tickets already come to the JS API consumer with the prefix added, and export tickets are created with this on the server (see ExportTicketHelper.exportIdToBytes()) and on the client (see ClientConfiguration.newTicketRaw()). This is very consistently applied and not at all magic.
Client code calling this should never care about h/ prefixes, and can make up their own naming schemes with total disregard for the implementation details.
| array = TypedArray.SetArrayUnionType.of(bytes); | ||
| } | ||
| Uint8Array bytesWithPrefix = new Uint8Array(length + 2); | ||
| // Add the shared ticket prefix at the start of the provided value |
There was a problem hiding this comment.
Please either put the prefix in a constant and/or comment linking it to where this prefix is defined. Not having a link is just going to be a headache for future maintenance, and as an outside observer, this is indeed magic.
| // Add the shared ticket prefix at the start of the provided value | |
| // Add the shared ticket prefix at the start of the provided value | |
| // Shared ticket prefix is defined in {@link io.deephaven.proto.util.SharedTicketHelper#TICKET_PREFIX} |
There was a problem hiding this comment.
I've moved all client-side ticket creation to a single class, with a caveat that this is by definition non-exhaustive, with references to the separate server side classes (in dhc, obviously cant link to dhe) where ticket prefixes are defined, registered.
I've also made some attempts to rename to be more specific about how these are used, but some of this code comes from DHE and can't really be rebuilt easily without larger changes - TableTicket should be something that is explicitly not a table, and is an export, probably should be part of ServerObject, but that in turn would require changing how ClientTableState and StateCache works, which would increase the scope dramatically.
Let me know if you think additional changes would help here.
niloc132
force-pushed
the
5666-shared-tickets
branch
from
5f0eff8 to
7e88120
Compare
There was a problem hiding this comment.
Looks good to me. I tested with two browser consoles that each had a Python IDE session. Tested that both regular tables and deephaven.ui widgets could be shared correctly. In the first console:
await session.runCode(`
from deephaven import empty_table, ui
t = empty_table(100).update("x=i")
uit = ui.table(t)
`)
var t = await session.getObject({ name: "t", type: "Table" })
var uit = await session.getObject({ name: "uit", type: "deephaven.ui.Element" })
await session.shareObject(t, "bender-sharing-t")
await session.shareObject(uit, "bender-sharing-uit")Then in the second console:
var t = await session.getSharedObject("bender-sharing-t", "Table")
var uit = await session.getSharedObject("bender-sharing-uit", "deephaven.ui.Element")
var cleanup = uit.addEventListener('message', event => {
console.log('event received', event, event.detail.getDataAsString(), event.detail.exportedObjects)
})
var result = await uit.sendMessage(JSON.stringify({ id: "fooo", jsonrpc: "2.0", "method": "setState", "params": [{}]}))
Made sure the result was exporting the table object from uit correctly, and t was working as expected.
Adds public methods on IdeConnection and IdeSession to create and dereference shared tickets.
Also includes a refator of ticket-creation/handling code to a single class in the JS API, with notes that this is incomplete by design - a client can never comprehensively list all ticket types, since they are pluggable.
Fixes #5666