0.17.0 version breaking Client injector interface

[avkonst]

The SSE work introduced the breaking API change which I did not notice:
from:

export interface Client {
  request(
    path: string,
    body: string,
    headers: Record<string, string>,
    options?: RequestOptions,
  ): Promise<[number, string]>;
}

to:

export interface Client {
  request(
    path: string,
    body: string,
    headers: Record<string, string>,
    options?: RequestOptions,
  ): Promise<Response>;
}

Our options to converge are:

1. Request => Response. Most flexible but too generic for implementors to implement, and leaky abstraction, eg. lets to hijack access to response headers, which we agreed to do type safe way via codegen. Also, complicates introspection of Requests and Responses very difficult.

export interface Client {
  request(
    req: Request,
  ): Promise<Response>;
}

2. as before but with possible ReadableStream<string> on return type. Restricted explicitly set expectations for implementors AND non breaking! Cons: an implementor should explicitly turn to returning the stream for enabling support for notifications, i.e. would not be trapped at compile time. Problem there is no way to know when stream is expected, when complete is expected if a client implementor needs to introspect the response data.

export interface Client {
  request(
    path: string,
    body: string,
    headers: Record<string, string>,
    options?: RequestOptions,
  ): Promise<[number, string | ReadableStream<string>]>;
}

3. as before but return stream of text, where a simple request (non streaming) would return a stream of one message and then closed stream). Breaking change, but not too much, easier to implement. Easier to hook introspection for the data. Stronger contract, not leaky.

export interface Client {
  request(
    path: string,
    body: string,
    headers: Record<string, string>,
    options?: RequestOptions,
  ): Promise<[number, ReadableStream<string>]>;
}

I think I prefer option 3.

[avkonst]

@andyyu2004 FYI. If you are interested in to participate

[avkonst]

cc @hardbyte

[avkonst]

other thing to consider here: if we decide to turn on msgpack encoding in the future, we would prefer to deal with bytes, rather than text.

[avkonst]

option 1 is also viable if no content introspection is needed by the implementor. Currently it is not needed anywhere in Partly code base. We only inject transport implementor, like playwright, for recording traces.

[avkonst]

option 4 from @hardbyte , which explicitly declares what we care about.

export interface RequestOptions {
    signal?: AbortSignal;
  }

  export interface ClientRequest {
    path: string;
    method: "POST";
    headers: Record<string, string>;
    body: string;
    signal?: AbortSignal;
  }

  export interface ClientResponse {
    status: number;
    headers: Headers;
    body: ReadableStream<Uint8Array> | null;
    text(): Promise<string>;
  }

  export interface Client {
    request(request: ClientRequest): Promise<ClientResponse>;
  }

[andyyu2004]

I considered doing option 4, and the last option I considered was having two methods, one for streaming one not. I don't really understand the downsides of what was merged though

[avkonst]

asymmetry: request in parts as parameters (locked down to what we care about), return type as obj (wider interface).

[avkonst]

option 5 from Andy: 2 methods both returning different types

[andyyu2004]

Happy to implement option 4 as Pick<Response, 'path' | 'method' | ...>. That seems like the best compromise

[avkonst]

Taking into account we might want to move to msgpack encoding for performance in the future, I am leaning towards more future proof option 1 + adjusten documentation of what we expect to pass and receive.

[avkonst]

ok, pick of props is also a good option, but I would converge to assuming bytes (body()) stream, rather than text(), which would work now for both cases + future proof for msgpack

[hardbyte]

I think given all this I'd update my "option 4" suggestion to be byte oriented:

  export interface RequestOptions {
    signal?: AbortSignal;
  }

  export interface ClientRequest {
    path: string;
    method: "POST";
    headers: Record<string, string>;
    body: Uint8Array;
    signal?: AbortSignal;
  }

  export interface ClientResponse {
    status: number;
    headers: Pick<Headers, "get">;
    body: ReadableStream<Uint8Array> | null;
  }

  export interface Client {
    request(request: ClientRequest): Promise<ClientResponse>;
  }

[hardbyte]

I'd go a step further and not Pick<Headers, "get"> either. Exposing an owned type makes the intent clearer in my opinion and we'll have to make this consistent for Rust and Python so relying on the DOM/JS objects at all.

  export interface ClientHeaders {
    get(name: string): string | null;
  }