Add data_converter features in Typescript (#81) - #286
Conversation
Signed-off-by: Antonio Lain <antonio.lain@temporal.io>
Merge remote-tracking branch 'upstream/main' into data-converter-ts
bergundy
left a comment
bergundy
left a comment
There was a problem hiding this comment.
Had a few notes, nothing major.
Should be good to merge once comments are addressed.
| import * as proto from '@temporalio/proto'; | ||
| import * as assert from 'assert'; | ||
|
|
||
| // Inject Buffer and Uint8Array into isolate to workaround SDK bug |
There was a problem hiding this comment.
| // Inject Buffer and Uint8Array into isolate to workaround SDK bug | |
| // Inject Buffer and Uint8Array from the node context to the workflow context to workaround SDK bug |
| // Inject Buffer and Uint8Array into isolate to workaround SDK bug | ||
| // TODO(antlai-temporal) Remove when SDK bug is fixed | ||
| const g = globalThis as any; | ||
| g.Uint8Array = g.constructor.constructor('return globalThis.Uint8Array')(); |
There was a problem hiding this comment.
I don't think you need this here the injection here, AFAIU, it's only needed for json protobuf.
There was a problem hiding this comment.
I still need to inject Uint8Array (not Buffer) because protobufjs does an instanceof check for Uint8Array when decoding (see reader.js:45 in protobufjs)
| async checkResult(runner, handle) { | ||
| // verify client result is DataBlob `0xdeadbeef` | ||
| const result = await handle.result(); | ||
| assert.deepEqual(result.toJSON(), expectedResult.toJSON()); |
There was a problem hiding this comment.
Hmm.. do you have to convert to json for the comparison?
There was a problem hiding this comment.
Chad's comment in the go version was not to compare protobuf directly, because it is a bit brittle, and use proto.Equal instead. There is no equivalent in protobufjs afaik, so I was just trying to make the check a bit more robust...
There was a problem hiding this comment.
I don't think that applies here, you can remove if deepEqual works as expected.
| assert.deepEqual(resultInHistory.toJSON(), expectedResult.toJSON()); | ||
|
|
||
| // get argument payload of WorkflowExecutionStarted event from workflow history | ||
| const payloadArg = await runner.getWorkflowArgumentPayload(handle); |
There was a problem hiding this comment.
Should we do the assertion on the metadata for the arg too?
There was a problem hiding this comment.
Good point, adding it now
|
|
||
| const toBase64 = (inArray: Uint8Array): Uint8Array => { | ||
| const buf = Buffer.from(inArray); | ||
| return encode(buf.toString('base64')); |
There was a problem hiding this comment.
nit: If you're already injecting Buffer, you can use Buffer.from(buf.toString('base64'), 'ascii')
There was a problem hiding this comment.
Much simpler, no need to worry about UTF8 in base64...
| import * as assert from 'assert'; | ||
| import { METADATA_ENCODING_KEY, Payload, PayloadCodec, ValueError } from '@temporalio/common'; | ||
| import * as proto from '@temporalio/proto'; | ||
| import { decode, encode } from '@temporalio/common/lib/encoding'; |
There was a problem hiding this comment.
@mjameswh I'm considering exporting this from common directly but let's see if we want to inject Buffer or not first.
| // Inject Buffer and Uint8Array into isolate to workaround SDK bug | ||
| // TODO(antlai-temporal) Remove workaround when SDK bug is fixed | ||
| const g = globalThis as any; | ||
| g.Uint8Array = g.constructor.constructor('return globalThis.Uint8Array')(); |
There was a problem hiding this comment.
Don't think you'll need to inject Uint8Array here, just Buffer AFAIR.
There was a problem hiding this comment.
You are right, it just needs Buffer
There was a problem hiding this comment.
Can you test passing a binary argument too please?
There was a problem hiding this comment.
I didn't want to leave some languages in /binary with argument, and others without it. Once I finish the python and java, I'll do one more PR to change all the languages in the binary to add the argument, if that's OK...
Both protobuf data converter features reached into the Node realm to overwrite a global inside the workflow sandbox: binary_protobuf replaced Uint8Array, and json_protobuf replaced Buffer. They date back to #286 and reference an unnamed SDK bug around how `bytes` fields cross the sandbox boundary. The SDK now normalizes protobufjs's `Buffer` allocations to `Uint8Array` when decoding, so neither injection is needed; both features pass without them.
2 participants
What was changed
Adding Typescript feature implementations for data converters json, json_protobuf, binary_protobuf, and custom codec
Checklist
Progress towards [Feature Request] Implement data_converter features #81