Review API from Revamp PR #107
Conversation
| public sealed class ClientConfig : UiPath.Ipc.EndpointConfig, System.IEquatable<UiPath.Ipc.ClientConfig> | ||
| { | ||
| public ClientConfig() { } | ||
| public string DebugName { get; set; } |
There was a problem hiding this comment.
not clear what DebugName does.
There was a problem hiding this comment.
It's a CoreIpc-traditional tagging mechanism for reading logs and understanding who's who.
I didn't like how Name sounded (it seemed more functional than it actually was), so I prefixed it with Debug.
On support/v21.10 the public ones were ListenerSettings.Name and Connection.Name, both of which are public, and which spin off into internals (i.e. ServiceClient inherits the Name from it's associated Listener if it's of the callback sort) and all end up in logs, from hundreds of places, i.e: this
| { | ||
| protected ClientTransport() { } | ||
| public abstract UiPath.Ipc.IClientState CreateState(); | ||
| public abstract void Validate(); |
There was a problem hiding this comment.
maybe we should think if we really need the Validate feature?
| public System.TimeSpan RequestTimeout { get; init; } | ||
| } | ||
| [System.Serializable] | ||
| public sealed class EndpointNotFoundException : System.ArgumentOutOfRangeException |
There was a problem hiding this comment.
not an argument out of range problem
argument out of range is if we could know statically that the argument is outside of the input domain
| public abstract class ListenerConfig : UiPath.Ipc.EndpointConfig, System.IEquatable<UiPath.Ipc.ListenerConfig> | ||
| { | ||
| protected ListenerConfig() { } | ||
| public System.Security.Cryptography.X509Certificates.X509Certificate? Certificate { get; init; } |
There was a problem hiding this comment.
was this in CoreIpc?
why do we expose this? is it for websocket only?
There was a problem hiding this comment.
Yes it was, and it was agnostic than merely websockets;
I was wondering the same thing.
We use CoreIpc over Tcp/Ipc, but I can't find a precise usage for this certificate thing.
We have 0 websocket usage.
| [return: System.Diagnostics.CodeAnalysis.NotNullIfNotNull("exception")] | ||
| public static UiPath.Ipc.Error? FromException(System.Exception? exception) { } | ||
| } | ||
| public interface IClient |
There was a problem hiding this comment.
IClientConnection name would be better:
- we have the usecase in which we identify the same connection in hashset
- the callback is only valid for that client connection, not if recreated
There was a problem hiding this comment.
Sounds good, but we'll have to discuss whether what you said still holds.
| public UiPath.Ipc.IClient Client { get; set; } | ||
| [Newtonsoft.Json.JsonIgnore] | ||
| public System.TimeSpan RequestTimeout { get; set; } | ||
| public TCallbackInterface GetCallback<TCallbackInterface>() |
There was a problem hiding this comment.
maybe it wouldn't be that bad to force the user to go through IClient(Connection) 😓 . it tells you more e.g. how long the callback is valid, but mostly for the reduced surface area.
| "your compiler.", true)] | ||
| public IpcServer() { } | ||
| public UiPath.Ipc.EndpointCollection Endpoints { get; init; } | ||
| public System.Collections.Generic.IReadOnlyList<UiPath.Ipc.ListenerConfig> Listeners { get; init; } |
There was a problem hiding this comment.
I think we can improve the entity model to have better matching between client and server, and generally to simplify it.
The only real relationship differences I can see:
- A client has a single connection while a listener has many. I don't see any problem with the API regarding this.
- A server can have multiple transport configurations "listeners". Of note, we could drop this feature of multiple listeners. A user could simply create more servers calling the same configuration code.
I'd propose this model:
Endpoints = { Client, Server }
Endpoint = (IServiceProvider, Scheduler, RequestTimeout)
Client : Endpoint + (ClientTransport)
Server : Endpoint + (ServerTransport)
| } | ||
| namespace UiPath.Ipc.Extensibility | ||
| { | ||
| public interface IListenerConfig<TSelf, TListenerState, TConnectionState> |
There was a problem hiding this comment.
IListenerConfig vs ListenerConfig not obvious.
Why other namespace not obvious.
Maybe you'd want to move all transport stuff into UiPath.Ipc.Transport (client included), but I don't see the need.
Too many generics, one of the problems we wanted to avoid.
There was a problem hiding this comment.
I agree, and we need to discuss it a bit, but this is exclusively for the select few who will do (maybe) a Blazor JSInterop transport.
| "_accepter"})] | ||
| public void Start() { } | ||
| public System.Threading.Tasks.Task WaitForStart() { } | ||
| public System.Threading.Tasks.Task WaitForStop() { } |
There was a problem hiding this comment.
what;s the differentce between WaitForStop() and DisposeAsync()?
I'd like to avoid supporting the "restart" scenario.
| public System.Threading.Tasks.TaskScheduler? Scheduler { get; set; } | ||
| public object? ServiceInstance { get; } | ||
| public System.IServiceProvider? ServiceProvider { get; } | ||
| public void Validate() { } |
There was a problem hiding this comment.
I see you've added ServiceProvider and scheduler to IpcBase - so we'd be good to remove them from here, right?
eduard-dumitru
force-pushed
the
review_revamp_api
branch
from
1150dc4 to
9f3573b
Compare
- other simplifications
eduard-dumitru
force-pushed
the
review_revamp_api
branch
from
9f3573b to
0339141
Compare
- decommission WaitForStop
No description provided.