Comments (9)
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!
from gentle_rpc.
IMO that would be great. To sum it up I drafted an opinionated implementation here
from gentle_rpc.
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 )
from gentle_rpc.
Thank you for your nice words @dvv !
- I would be open to a PR here, because I am not very familiar with this topic.
- Now
respond
returns a universalResponse
object. I am certain this is the most framework agnostic way and would like to keep it this way. - I am not exactly sure what you mean. I am certain it acts spec confirm now. How would you change it?
- 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!
from gentle_rpc.
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
gentle_rpc/client/validation.ts
Line 50 in 7eb9df3
What do you think?
TIA
from gentle_rpc.
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 theok
message in line 144 toctx.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 infn
? - 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!
from gentle_rpc.
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
from gentle_rpc.
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.
from gentle_rpc.
Hi @dvv I created something out of your and my ideas. Maybe you wanna take a look at it: https://github.com/Zaubrik/schicksal
from gentle_rpc.
Related Issues (8)
- websocket support HOT 2
- Implement validation/schema
- generateId() should not rely on window to get crypto object because there is no window in worker HOT 2
- Error: The received data is no valid JSON-RPC 2.0 Response object. HOT 2
- Function call never yields result HOT 2
- "private fields are not currently supported" error in browser HOT 7
- new std/[email protected] API support HOT 3
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from gentle_rpc.