feat!: Custom data converter #477

lorensr · 2022-02-11T23:11:02Z

What was changed

Implemented custom data converter feature. Design doc: https://github.com/lorensr/sdk-node/blob/custom-data-converter/docs/data-converter.md
Separated common package into common (used everywhere but the workflow) and workflow-common (used everywhere) to reduce workflow bundle size.
Un-reverted Add binary/protobuf serialization and WorkerOptions.dataConverterPath #430

Sorry, I should have un-reverted in a subsequent PR so that it wouldn't be included in this PR diff. Here's a diff without that first un-revert commit:

lorensr/sdk-typescript@cc0d414...custom-data-converter

Checklist

Closes [Feature Request] Custom data converters #130
Closes [Feature Request] Protobuf payload converter support #237
How was this tested:

Draft PR: tests aren't passing yet, and I need to write more to cover codec.

Any docs updates needed?

Yep

… WorkerOptions.dataConverterPath (temporalio#430)"" This reverts commit 139550d.

export interface DataConverter { payloadConverterPath?: string; payloadCodec?: PayloadCodec; } export interface PayloadConverter { toPayload<T>(value: T): Payload; fromPayload<T>(payload: Payload): T; } export interface PayloadCodec { encode(payloads: Payload[]): Promise<Payload[]>; decode(payloads: Payload[]): Promise<Payload[]>; }

docs/data-converter.md

packages/common/src/codec-helpers.ts

packages/workflow-common/package.json

packages/workflow-common/src/converter/data-converter.ts

packages/workflow-common/src/converter/payload-converter.ts

bergundy · 2022-02-14T23:03:13Z

packages/workflow/package.json

+    "@temporalio/workflow-common": "file:../workflow-common"
+  },
+  "devDependencies": {
+    "@temporalio/proto": "file:../proto"


Is this a dev dependency now? Is it even a dependency anymore?

I added as devDep because we import type { coresdk } in a few places in this package

It should be a non dev dependency if we export any of those types or anything that uses those types (e.g. function signatures), to be safe, let's make it non dev.

…nverter

cretz · 2022-02-15T14:07:16Z

docs/protobuf-libraries.md

+
+- Most popular lib (5M downloads/wk)
+- Fairly inactive maintainers (infrequent updates, many open PRs & issues)
+- Non-standard JSON serialization


What makes it non-standard? I expect every message class to have a toJSON method (which I believe is basically toObject + JSON.stringify) and a fromObject which can be used after JSON.parse.

Every class has a toJSON method, but JSON.stringify() doesn't match the spec:

https://developers.google.com/protocol-buffers/docs/proto3#json

Eg stringify throws on BigInts and doesn't base64-encode binary. protobufjs hasn't implemented a number of the types that need custom encoding: protobufjs/protobuf.js#1304

Eg stringify throws on BigInts and doesn't base64-encode binary.

Why use stringify? Doesn't the toJSON directly work with these? Is there an issue to follow?

protobufjs hasn't implemented a number of the types that need custom encoding: protobufjs/protobuf.js#1304

Makes sense. They just need better support for well known types. I wonder if we can contribute? Otherwise, I wonder if we can walk the object for WKT's and convert them ourselves.

The last commit in their repo was in last May, I fear contribution is not a viable option.

Why use stringify? Doesn't the toJSON directly work with these? Is there an issue to follow?

I just meant that if toJSON were implemented with JSON.stringify, it wouldn't work. I made this issue: #481

I wonder if we can contribute? Otherwise, I wonder if we can walk the object for WKT's and convert them ourselves.

I'd like to try contributing. If converting ourselves, unless we fork protobufjs, we'd be doing what proto3-json-serializer does, and would have the same requirement on how the user generates & provides their messages.

👍 I am not sure it's reasonable to ask people to use our custom protoc. Ideally we can do this from the outside on protobufjs-generated classes/descriptors.

Having said that, this being the only one of our languages without a canonical, supported proto-JSON implementation, I'd support just erroring in data converters if protos are provided in either direction, but allow them to use a custom converter to pick their preferred conversion (or binary only, but we should still error on from-payload when we receive json/protobuf).

to ask people to use our custom protoc

We're still having them use the protobufjs generator (pbjs). It may not be the way they're currently running it. If not, I think it's straightforward to switch or to do both—our way for temporal code and their way for the rest of their code. The main difference in use is doing Message.create() instead of new Message().

If we think running the generator our way is too big an ask for some users, we could add support for just binary/protobuf for them, without removing current functionality.

the only one of our languages without a canonical, supported proto-JSON implementation

I think the library we're using is well-maintained and from Google/canonical: https://github.com/googleapis/proto3-json-serializer-nodejs

The main difference in use is doing Message.create() instead of new Message().

Can you document why in one of your docs? Is this a limitation of proto3-json-serializer?

If we think running the generator our way is too big an ask for some user

I don't think it's too big of an ask, just offering the alternative of having json/protobuf be opt-in instead of by default if it's too much trouble.

I think the library we're using is well-maintained and from Google/canonical: https://github.com/googleapis/proto3-json-serializer-nodejs

Sorry, to clarify, there's a reason that JS isn't listed at https://developers.google.com/protocol-buffers/docs/tutorials or in official docs. But I agree that JSON serializer works if we document how to use it.

Can you document why in one of your docs? Is this a limitation of proto3-json-serializer?

Yes. Limitation of this way of running pbjs.

lorensr · 2022-03-01T04:19:11Z

Addressed review comments and:

Fixed and added tests
Stopped encoding/decoding headers (pending further discussion, will probably re-enable and add to otel in future PR)
Restructured common packages to:

common
internal-workflow-common
internal-non-workflow-common

where the first exports things you might want to use in your own common code (shared between client/worker/workflow) like data converters, failures, and errors.

Sushisource

Like this new design! Didn't look to much into gritty details but just some comments on high-level stuff.

packages/common/src/converter/payload-codec.ts

packages/common/src/converter/payload-converter.ts

packages/worker/src/workflow-codec-runner.ts

bergundy

Didn't have many comments, most of the critical ones I communicated on slack.
I want to try to use TS type checking to ensure that we always encode and decode payloads, but it doesn't block this PR.

I'm approving, this is good to merge as soon as you go over the comments.

docs/data-converter.md

packages/client/src/workflow-client.ts

packages/common/src/converter/data-converter.ts

packages/common/src/converter/payload-converter.ts

packages/test/src/test-custom-payload-codec.ts

bergundy · 2022-03-02T01:10:36Z

packages/test/src/test-custom-payload-codec.ts

+    };
+    await Promise.all([worker.run(), runAndShutdown()]);
+    t.is(workflowLogs[0], 'encoded');
+    t.is(activityLogs[0], 'Activityencodedencoded');


nit: You can probably test this without the logger if you use a single codec for both encoding and decoding.

packages/worker/src/worker.ts

bergundy · 2022-03-02T01:22:14Z

packages/workflow/package.json

+    "@temporalio/workflow-common": "file:../workflow-common"
+  },
+  "devDependencies": {
+    "@temporalio/proto": "file:../proto"


It should be a non dev dependency if we export any of those types or anything that uses those types (e.g. function signatures), to be safe, let's make it non dev.

packages/workflow/src/worker-interface.ts

docs/data-converter.md

packages/common/src/converter/payload-converter.ts

packages/test/src/mock-native-worker.ts

lorensr · 2022-03-02T20:40:18Z

I'll update sdk-features

lorensr added 10 commits January 22, 2022 00:48

Revert "Revert "feat: Add Protobuf binary and JSON data converter and…

fd9978f

… WorkerOptions.dataConverterPath (temporalio#430)"" This reverts commit 139550d.

fix: Make node assert module available to webpack

cc0d414

test(worker): Add failing proto integration test

158b3bb

feat: Use custom data converter in Workflow

dcf79d4

chore: Create workflow-common package, fixes temporalio#434

629abe5

feat: Only include proto3-json-serializer when needed

3e501b0

chore: Remove unused data converter from otel interceptors

e0152bb

feat: Complete new DC API conversion

3430625

docs: Update docs/data-converter.md; fix linting

9720e70

bergundy reviewed Feb 14, 2022

View reviewed changes

Use fallback instead of empty module for __temporal_custom_payload_co…

9b474d1

…nverter

cretz reviewed Feb 15, 2022

View reviewed changes

lorensr added 17 commits February 15, 2022 17:57

docs: Add docstrings to exported functions

717e096

chore: Use root .gitignore

01090b7

chore: Remove duplicate devDep

bb62659

chore: Address review comments

1d51cff

fix: Don't mutate Failures

b809ccb

fix: Don't mutate Activation when decoding

9bb131c

fix: Run errorToFailure inside vm

c35ed2a

chore: Don't export proto payload converters from workflow-common

62c4885

fix: Update tryToContinueAfterCompletion

6cb9540

chore: Move common packages

0bc2fda

feat!: Add common package and update references

1b265a1

fix: UndefinedPayloadConverter test

f37057c

chore: Fix tests

cd1a310

chore: Fix integration test and add codec tests

663ffbb

chore: Disable header encoding for now

a29118b

chore: Fix rest of integration tests

4a931b7

test: Run custom codec on integration tests

29ac6e6

lorensr marked this pull request as ready for review March 1, 2022 04:19

chore: Merge main

af61fff

lorensr requested a review from bergundy March 1, 2022 05:46

lorensr added 2 commits March 1, 2022 15:58

Remove unnecessary useCustomPayloadConverter

a68bc36

Guard against non-Record module

c25e9ab

Sushisource reviewed Mar 1, 2022

View reviewed changes

packages/common/src/converter/payload-codec.ts Outdated Show resolved Hide resolved

packages/common/src/converter/payload-converter.ts Outdated Show resolved Hide resolved

packages/worker/src/workflow-codec-runner.ts Show resolved Hide resolved

bergundy approved these changes Mar 2, 2022

View reviewed changes

lorensr added 2 commits March 2, 2022 11:27

Make PayloadConverter.toPayload return undefined instead of throwing

ffcef1c

Address review comments

5b4ded8

bergundy approved these changes Mar 2, 2022

View reviewed changes

Address comments

243f934

lorensr merged commit 82351cc into temporalio:main Mar 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Custom data converter #477

feat!: Custom data converter #477

lorensr commented Feb 11, 2022 •

edited by bergundy

Loading

bergundy Feb 14, 2022

lorensr Feb 18, 2022

bergundy Mar 2, 2022

cretz Feb 15, 2022 •

edited

Loading

lorensr Feb 15, 2022

cretz Feb 15, 2022

bergundy Feb 15, 2022

lorensr Feb 15, 2022

cretz Feb 15, 2022 •

edited

Loading

lorensr Feb 15, 2022 •

edited

Loading

cretz Feb 15, 2022

lorensr Feb 15, 2022

lorensr commented Mar 1, 2022

Sushisource left a comment

bergundy left a comment

bergundy Mar 2, 2022

bergundy Mar 2, 2022

lorensr commented Mar 2, 2022

feat!: Custom data converter #477

feat!: Custom data converter #477

Conversation

lorensr commented Feb 11, 2022 • edited by bergundy Loading

What was changed

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Feb 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Feb 15, 2022 • edited Loading

Choose a reason for hiding this comment

lorensr Feb 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorensr commented Mar 1, 2022

Sushisource left a comment

Choose a reason for hiding this comment

bergundy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorensr commented Mar 2, 2022

lorensr commented Feb 11, 2022 •

edited by bergundy

Loading

cretz Feb 15, 2022 •

edited

Loading

cretz Feb 15, 2022 •

edited

Loading

lorensr Feb 15, 2022 •

edited

Loading