Feature request

[dvv]

Hi!

Thank you for an excellent library first!

Now I'm trying to use the lib and found a few corner cases I would like to point out:

1. Hardcoded JSON parse/stringify. I wonder if we could delegate them to an optional transformer (superjson?)
2. Hardcoded request interface. I use oak. For reference, the converter could be

   router
     .post("JSONRPC", "/rpc", async (ctx) => {
       const req = {
         headers: ctx.request.headers,
         async text() {
           return await (ctx.request.body({ type: "text" }).value).catch(() => "")
         },
       }
       const res = await respond(rpcMethods, req as Request, {
         additionalArguments: [{ args: { _ctx: ctx.state }, allMethods: true }],
         // publicErrorStack: true,
       })
       ctx.response.status = res.status
       res.headers.forEach((value, key) => ctx.response.headers.set(key, value))
       ctx.response.body = res.body
     })
   

3. Way rude batch behaviour. It throws at the first error as if the batch is a DB transaction:

   try { console.log(await remote.batch({ nonExisting1: ["nonExisting1"], existing: ["existing"] })) } catch (e) { console.error({ m: e.message, c: e.code, d: e.data, i: e.id }) }
   

4. No hook available to operate responses. Say think of error logging/mangling/muting.

What do you think? Feasible to be fixed in library or should be left to app level?

TIA

[timonson]

Thank you for your nice words @dvv !

1. I would be open to a PR here, because I am not very familiar with this topic.
2. Now respond returns a universal Response object. I am certain this is the most framework agnostic way and would like to keep it this way.
3. I am not exactly sure what you mean. I am certain it acts spec confirm now. How would you change it?
4. This is definitely what I would consider. If you already have a good idea, I would be happy to accept a PR here, too.

Thank you very much!

[dvv]

Aha.

On 3: The point is to decide on how you treat the spec's "SHOULD" in "A Response object SHOULD exist for each Request object":

To send several Request objects at the same time, the Client MAY send an Array filled with Request objects. The Server should respond with an Array containing the corresponding Response objects, after all of the batch Request objects have been processed. A Response object SHOULD exist for each Request object, except that there SHOULD NOT be any Response objects for notifications. The Server MAY process a batch rpc call as a set of concurrent tasks, processing them in any order and with any width of parallelism.

So now if I call a batch of two requests of which the first is failing I receive no more that the first error thrown. I would instead like to receive a batch of two responses: [new RpcError(...the-error-from-the-first-request...), ...result-of-the-second-request...].

I propose to:

• keep the single-request call to either return result or throw the error
• change the batch call to return batch results/errors and throw only on invalid batch

The change could be narrowed to here https://github.com/timonson/gentle_rpc/blob/7eb9df3fdc4035346c534845ad92371e0a5b92ca/client/validation.ts#L50

What do you think?

TIA

[timonson]

That's really an interesting proposal because recently I was in a situation where I would have needed exactly that. I wanted to send a batch of emails via SMTP and your proposal would have prevented me from sending some duplicates!

What would you say if we delete a bunch of code and make this library even more pragmatic by just returning the RPC response objects/errors, meaning the type RpcResponse.

I believe more users could adopt this library and build more impactful stuff on top of it. I would really appreciate your help, if your willing to.

Thanks!

[dvv]

IMO that would be great. To sum it up I drafted an opinionated implementation here

[timonson]

This is really a shockingly amazing work, especially considering the short time you needed @dvv ! I would like to ask a couple of questions and point out a few minor errors:

• Line 41: Why is the order important here?
• Line 46: method doesn't need to be optional, I believe.
• Line 50: Would you mind removing the underscores in the strings? I would prefer it because error messages in JS typically don't have underscores. According to line 134 we also should add validation to the union type. And I would like to add the ok message in line 144 to ctx.meta.error somewhere. What do you think?
• Line 71: I would suggest that we remove .#stringify completely, because in most cases the parsed data is needed.
• Line 91: Why do you add the value property?
• Line 93: I think it should be input.params
• Line 124: Would you agree to removing .authorize completely or do you have a special use case in mind which could not be solved in fn?
• Line 127: Can you tell me more about the Metrics object?
• Line 130: I believe that ctx should not be part of the two arrays.

I am really very glad that you did this and I would be more than happy to accept a PR. Please let me know what you think about my proposed changes. Thank you!

[dvv]

Thanks!

Line 41: Why is the order important here?

JsonRpcSuccessSchema's result is unknown. If we put it the first, JsonRpcResponseSchema will always infer as JsonRpcSuccessSchema as objects without result, despite having error or not, will always match. NB: I've been seeing TypeScript for a week, hence such maybe naive hacks. The rationale here is not introduce artificial discrimination fields and not limit the result type so it can be anything. That's then the job of serializer to put it on the wire.

Line 46: method doesn't need to be optional, I believe.

It's assigned conditionally at L97 so may de undefined.

Line 50: Would you mind removing the underscores in the strings?

The rationale here was to make this field a pure machine tag to classify error (in the logger). But it certainly can be anything you like.

Line 71: I would suggest that we remove .#stringify completely

It's used only by high-level handleJson as potentially overrideable (if marked protected) custom serializer. To process already parsed data and receive JS result it's supposed to use handleRequest.

Line 91: Why do you add the value property?

It's not to spoil parent context's meta in case it comes from prototype.

Line 93: I think it should be input.params

The idea is progressively improve the metadata info quality. Here we only know the raw input. Should the input be validated successfully, we replace it with validated version at L102 etc. Similar thoughts apply to putting return value/error.

Line 127: Can you tell me more about the Metrics object?

I tried to employ https://deno.land/x/ts_prometheus but it should certainly be thrown away in library mode.

Line 130: I believe that ctx should not be part of the two arrays.

The first case [...request.params, ctx] is for arrayish params: [1, 2] -> [1, 2, ctx] The second [request.params, ctx] is for the single-object params: { a: 1, b: 2 } -> [{ a: 1, b: 2 }, ctx] That ^^^ is (was?) the plan to have predictable contract of the implementation hook.

What do you think? TIA

[timonson]

Thank you very much!

JsonRpcSuccessSchema's result is unknown. If we put it the first, JsonRpcResponseSchema will always infer as JsonRpcSuccessSchema as objects without result, despite having error or not, will always match.

I am not very familar with zod but as far as I understand it, JsonRpcSuccessSchema has a result property and JsonRpcFailureSchema has an error property. Why does it matter that result maybe unknown?

It's assigned conditionally at L97 so may de undefined.

To me it looks like it is not optional. Am I missing something?

const JsonRpcRequestSchema = JsonRpcBaseSchema.merge(JsonRpcCallSchema).extend({
  id: JsonRpcIdSchema.optional(),
});

It's used only by high-level handleJson as potentially overrideable (if marked protected) custom serializer. To process already parsed data and receive JS result it's supposed to use handleRequest.

I would remove the method #stringify because stringified data is not required internally and the users would prefer parsed data as return value.

It's not to spoil parent context's meta in case it comes from prototype.

Could you explain to me please where the parent's ctx comes from? Isn't ctx created each time handleRequest is called? I would also suggest using a class instead of Object.create, because then people could extend ctx easily. For example:

interface Meta {
  method?: string;
  args?: unknown;
  result?: unknown;
  duration_ms?: number;
  error?:
    | "method not found"
    | "invalid arguments"
    | "invalid return type"
    | "exception"
    | unknown;
  stack?: unknown;
}

class JsonRpcContext {
  meta: Meta = {};
}

The idea is progressively improve the metadata info quality. Here we only know the raw input. Should the input be validated successfully, we replace it with validated version at L102 etc. Similar thoughts apply to putting return value/error.

It looks very confusing to me that c.meta.args is assigned the whole input or only a part of it ri.data.params.

That ^^^ is (was?) the plan to have predictable contract of the implementation hook.

This is really a very elegant way to pass the different arguments. But you already passed ctx to handler.fn.apply as first argument. I would be surprised if you couldn't remove ctx from the arrays.

[dvv]

Why does it matter that result maybe unknown?

I believe because unknown includes undefined hence absence of result would always mean success despite presence of error.

To me it looks like it is not optional

I believe ids are optional. Not provided id means a notification is sent.

I would remove the method #stringify

This is all about custom JSON reviver/replacer. Notice the special case at https://gist.github.com/dvv/4bf04c824678362b1c9c2c63fa3ab7b4#file-jsonrpc-ts-L71

Below is how to use it (and let it decide on wire format):

export const protocolHttpRouter = new oak.Router<AppState>()
  .post("/rpc", async ctx => {
    const body = await (ctx.request.body({ type: "text" }).value).catch(_e => "")
    ctx.response.body = await getRpc().handleJson(body, ctx.state)
  })

where the parent's ctx comes from? Isn't ctx created each time handleRequest is called?

In my case from the oak state. And right, it is an instance of a class AppState. I would like to share it across the request but keep meta per method invocation. I mean a request can consist of a bunch of method calls each of which has own meta initially come from request meta. If it's a bit opinionated feel free to change that.

It looks very confusing to me that c.meta.args is assigned the whole input or only a part of it ri.data.params.

I use meta to progressively collect stuff concerning a method call. During the execution there's a bunch of points where execution can terminate and I would like to pull the most of what could be pulled. Say here https://gist.github.com/dvv/4bf04c824678362b1c9c2c63fa3ab7b4#file-jsonrpc-ts-L93 we only know a raw request object. We don't yet know even the method because we haven't yet validated that request object. Say it doesn't validate hence our latest knowledge is just that raw request object. If we arrived at https://gist.github.com/dvv/4bf04c824678362b1c9c2c63fa3ab7b4#file-jsonrpc-ts-L97 we are certain the request object is valid hence we may store it's method and params. If later the execution terminates for any reason we would know the method and the params for sure. In the end of the day the log record will show the latest data we are certain about and the cause of request execution termination.

I would be surprised if you couldn't remove ctx from the arrays.

I could but I haven't yet decided on what a handler should be. In case of arrow function this could be a problem.

Thank you very much for the feedback! I must admit the code is very opinionated but if you get rid of the stuff you doubt in that could form a skeleton of a lib )

[timonson]

Hi @dvv I created something out of your and my ideas. Maybe you wanna take a look at it: https://github.com/Zaubrik/schicksal