-
Notifications
You must be signed in to change notification settings - Fork 52
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
fix: allow invoking with Uint8Array #947
Conversation
Niraj-Kamdar
commented
Jun 22, 2022
•
edited
Loading
edited
- allow calling invoke with Uint8Array
- use Uint8Array instead of ArrayBuffer for Bytes on Plugin side because msgpack always decode Bytes into Uint8Array
doesn't msgpack decode into Uint8Array? |
That's typo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm 🍏
edit: btw feel free to ignore my suggestion if you prefer the other way
const isEvenPlugin = () => { | ||
class IsEvenPlugin extends PluginModule<Record<string, unknown>> { | ||
public isEven(args: {num: number}, client: Client): boolean { | ||
return (args.num & 1) ? false : true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return (args.num & 1) ? false : true; | |
return !(args.num & 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I think we may be "attacking" the root problem in a wrong way here, let me explain.
Problem Statement
First, I think we need to be more descriptive of the exact problem we're solving here. I believe it is this...
schema.graphql
type Module {
method(arg: Bytes!): Bytes!
}
invoke.ts
// To make it easier for the caller, we should accept any type of arraybuffer
client.invoke({
method: "method",
args: {
arg: ArrayBuffer | Uint8Array | any ArrayBufferView
}
});
// We should also support passing in pre-encoded arguments
client.invoke({
method: "method",
args: argsBuf // ArrayBuffer
});
// We need to differentiate between two types of results:
// (1) A msgpack buffer that's not decoded
// (2) A buffer that's returned from the method
const msgpackBuf = client.invoke({
method: "method",
args: { ... },
noDecode: true
});
const resultBuf = client.invoke({
method: "method",
args: { ... },
});
Not to mention what happens when sub-invokes are used.
Solution
I'm not sure, but I don't think this PR addresses all of the cases above. I think we need to look into how we implement support for the noDecode
option, and see if it covers all cases here.
Hey @Niraj-Kamdar, I implemented a more holistic solution to the problem you're describing here in this PR: More details described here: https://discord.com/channels/796821176743362611/908544308876050515/991579335628701696 |