-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature request #17
Comments
Thank you for your nice words @dvv !
Thank you very much! |
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":
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 propose to:
The change could be narrowed to here gentle_rpc/client/validation.ts Line 50 in 7eb9df3
What do you think? TIA |
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 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! |
IMO that would be great. To sum it up I drafted an opinionated implementation here |
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:
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! |
Thanks!
It's assigned conditionally at L97 so may de undefined.
The rationale here was to make this field a pure machine tag to classify error (in the logger).
It's used only by high-level
It's not to spoil parent context's meta in case it comes from prototype.
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.
I tried to employ https://deno.land/x/ts_prometheus but it should certainly be thrown away in library mode.
The first case What do you think? |
Thank you very much!
I am not very familar with zod but as far as I understand it,
To me it looks like it is not optional. Am I missing something? const JsonRpcRequestSchema = JsonRpcBaseSchema.merge(JsonRpcCallSchema).extend({
id: JsonRpcIdSchema.optional(),
});
I would remove the method
Could you explain to me please where the parent's 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 = {};
}
It looks very confusing to me that
This is really a very elegant way to pass the different arguments. But you |
I believe because
I believe ids are optional. Not provided id means a notification is sent.
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)
})
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.
I use meta to progressively collect stuff concerning a method call.
I could but I haven't yet decided on what a handler should be. In case of arrow function 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 ) |
Hi @dvv I created something out of your and my ideas. Maybe you wanna take a look at it: https://github.com/Zaubrik/schicksal |
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:
What do you think? Feasible to be fixed in library or should be left to app level?
TIA
The text was updated successfully, but these errors were encountered: