tatethurston/TwirpScript

Why was `url` property removed from client middleware?

Closed this issue · 3 comments

In #29 the url property from interface MiddlewareConfig was removed. Any specific reason for this? I was using it in my app and now i cannot check which url the request is being sent to.

Example:

client.use((config, next) => {
  const auth = localStorage.getItem("auth");
  if (auth && !config.url?.includes("auth")) {
    config.headers["authorization"] = `bearer ${auth}`;
  }
  return next(config);
});

Hey @antonioorct thanks for reporting this. First off, I apologize for the thrash here.

My intention was to replace url with baseURL, prefix and path for API consistency:

interface MiddlewareConfig {
  /**
   * The base URL for the RPC. The service path will be appended to this string.
   */
  baseURL: string;
  /**
   * HTTP headers to include in the RPC.
   */
  headers: Record<string, string>;
  /**
   * A path prefix such as "/my/custom/prefix". Defaults to "/twirp", but can be set to "".
   */
  prefix: string;
  /**
   * The URL path (excluding the prefix) for the RPC.
   */
  path: string;
}

I see overlooked adding path in #29 !

I'm curious on your thoughts on the developer experience here though. Do you have a preference for interacting with the distinct parts (baseURL, prefix and path) vs url?

I'm curious if preserving the distinct parts simply adds cognitive overhead when working with the full url is sufficient (and maybe more ergonomic!)

The later would look like what you were using prior to #29:

interface MiddlewareConfig {
  /**
   * HTTP headers to include in the RPC.
   */
  headers: Record<string, string>;
  /**
   * The URL for the RPC.
   */
  url: string;
}

@tatethurston IMO, having the url split into its corresponding parts is much more intuitive and allows for more fine grained searching, specifically in my example i only need the path part of the url. And in general i doubt you'd ever need to search the url as a whole.

@antonioorct would having the ‘service’ and ‘method’ be more helpful than the path? I could see having that available as readonly fields in the config/context object (eg service: “Haberdasher”, method: “MakeHat”).