refactor(minimal ops) refactor minimal ops & rename to buffer ops #9719
Conversation
| R: ValueOrVector, | ||
| { | ||
| rt.register_op(name, metrics_op(name, buffer_op_sync(op_fn))); | ||
| } |
There was a problem hiding this comment.
I like how this is mirrors the structure of the JSON ops register function - so I think easier to understand.
|
@ry I made the required changes and added some tests. |
There was a problem hiding this comment.
A few more nitpicks from me, but after that I think we're ready to land this PR, great work @inteon
| function opAsync(opName, opRequestBuilder, opResultParser) { | ||
| // Make sure requests of this type are handled by the asyncHandler | ||
| // The asyncHandler's role is to call the "promiseTable[requestId]" function | ||
| core.setAsyncHandlerByName(opName, (bufUi8, _) => { |
There was a problem hiding this comment.
Not entirely sure, but I have a feeling that using arrow function here might be less performant than having a named function definition before opAsync
There was a problem hiding this comment.
opResultParser is passed into this arrow function, so that makes it hard to extract this arrow function from this opAsync function
There was a problem hiding this comment.
a solution could be to call this function on an object that stores this opResultParser function, but I found this quickly makes the code a lot less readable. please let me know if you think this is useful (I could create such a solution and share it with you for review)
There was a problem hiding this comment.
@inteon is it required to pass that opResultParser? I think you could just directly call it in such function
There was a problem hiding this comment.
In #9457, instead of passing bufferOpParseResult as argument, I pass jsonOpParseResult. This way code can be shared between jsonOps and bufferOps.
https://github.com/inteon/deno/blob/2f9a9cabd0d7c48f440677ff21885eac30d22ce3/core/core.js#L320-L334
https://github.com/inteon/deno/blob/2f9a9cabd0d7c48f440677ff21885eac30d22ce3/core/core.js#L364-L378
There was a problem hiding this comment.
I see, let's land it as is then.
| function opAsync(opName, opRequestBuilder, opResultParser) { | ||
| // Make sure requests of this type are handled by the asyncHandler | ||
| // The asyncHandler's role is to call the "promiseTable[requestId]" function | ||
| core.setAsyncHandlerByName(opName, (bufUi8, _) => { |
There was a problem hiding this comment.
Also wondering if this should be called only once, otherwise a new callback is set with each call to the op
Extracted minimal ops rewrite from #9457.
Is first step in effort to merge json and buffer ops.