Allow studio ui to set globals + expose session state returned by the server #651
Conversation
packages/driver/src/fetchConn.ts
Outdated
| @@ -143,6 +143,8 @@ class BaseFetchConnection extends BaseRawConnection { | |||
| } | |||
|
|
|||
| export class AdminUIFetchConnection extends BaseFetchConnection { | |||
| cliMode = true; | |||
There was a problem hiding this comment.
What is the relationship between the names cli and AdminUI here? Can/should this flag be named adminUiMode or are there some other cli commands that would also run in this mode in the future?
There was a problem hiding this comment.
The name cliMode was because it opts in to the cli behaviour where we actually read any updated session state the server returns instead of discarding it (though in theory it should never return updated state anyway for normal client usage). But we can call it adminUIMode if that makes more sense. It's a private api, only going to be used by the ui anyway.
There was a problem hiding this comment.
Not a blocker, for sure. Other options: emulateCli, useCliBehavior, etc. Definitely not a hard requirement and adminUiMode isn't much more enlightening than cliMode now that you've explained it.
There was a problem hiding this comment.
I think adminUiMode is probably the better name. I don't think we're likely to add anything else separate from the ui that will need the 'cli behaviour', and it's possible we'll need to add more hacks in the future for the ui that are not 'cli like'.
packages/driver/src/baseConn.ts
Outdated
| @@ -134,6 +140,9 @@ export class BaseRawConnection { | |||
|
|
|||
| protected stateCodec: ICodec = INVALID_CODEC; | |||
| protected stateCache: [Session, Uint8Array] | null = null; | |||
| protected lastStateUpdate: any = null; | |||
There was a problem hiding this comment.
Not a blocker: is there a more specific type we can use here? Since it doesn't look like we're accessing it at runtime, could we use unknown instead?
There was a problem hiding this comment.
The exact type depends on the schema, but yes, we could have something a bit more specific like:
{
globals?: {[name: string]: any},
config?: {[name: string]: any},
aliases?: [string, string][]
}
There was a problem hiding this comment.
Unless we need to use it in runtime code, my vote is for unknown since it's the simplest possible safe type here. Otherwise, if there is a common interface for this property that we might want to use in the future, now seems like a good time to give this a name while we're in here.
And, to reiterate, my preference for those record types would be Record<string, unknown> rather than { [name: string]: any } just to require downstream checking if anything ever did want to access a property.
There was a problem hiding this comment.
Yep, good point. It does share the type with the session state we send, which is currently typed as any. And aside from changing any -> unknown which makes sense, is there a reason to use Record<Key, Type> over {[key: Key]: Type}? For me the second seems more useful, since it can have a hint of what the key represents, eg. is it a mapping of 'names' or 'ids' to something.
There was a problem hiding this comment.
Oh, sure, I just thought name was a throwaway identifier 😆. I mostly prefer the Record syntax because it communicates to me that it's just a record rather than having some hidden mapped type magic that I should check for, so makes it a little easier to glance at. Not a big deal for me either way.
packages/driver/src/baseConn.ts
Outdated
| if (this.cliMode && stateTypeId === this.stateCodec.tid) { | ||
| this.lastStateUpdate = this.stateCodec.decode( | ||
| new ReadBuffer(stateData) | ||
| ); | ||
| } |
There was a problem hiding this comment.
It's possible that I just don't understand what the context of this feature is, but can I have a quick tl;dr on what these new properties are for?
There was a problem hiding this comment.
Some protocol context: Normally for clients the session state (ie. globals, aliases and session config (like apply_access_policies)) is managed entirely client side via the client.with* api's, then encoded and sent with every query (since the protocol is stateless except for transactions). However for the cli, there's obviously no programmatic api to set this state, so instead you use the set global/alias/module ... and configure session ... commands in edgeql, and when you run one of these commands the server returns an updated session state object, which the cli stores and sends back with following queries.
And the context for this PR: Currently the UI repl follows the client behaviour, where session state is set entirely in the UI. But we want to allow the edgeql commands to work, so this PR allows the private api's the ui uses to enable this hybrid client/cli behaviour by:
- Allowing the
rawParse/rawExecutemethods to set custom capabilities flags, so the server won't reject the edgeql commands - Actually decoding the updated state the server returns. I'm just storing this updated state in a
lastStateUpdateproperty on the connection (similar tolastStatus), mainly so I don't add any complexity by having to thread it through as a return value along with the actual result from the execute method. - Adding a
cliModeflag so the above decoding is only enabled for the ui. And also it disables an optimisation where we don't send the session state if nothing's been set, since we need the server to send an updated codec to decode the session state, as we actually need to read it now.
There was a problem hiding this comment.
Great, thanks for the additional context! 🙏
No description provided.