Support protobuf-es (@bufbuild/protobuf v2) payloads#2047
Conversation
wcygan
changed the title
wcygan
changed the title
|
Are there any reason why this would need to be part of the SDK itself? |
Hey @mjameswh I started by mirroring the existing layout. It might not be strictly necessary, but it might help with discoverability since the SDK already ships the protobufjs converter. I also reused some internal APIs like PayloadConverterWithEncoding in the new code. I opened this PR because I see myself reusing this stack with Rust / TypeScript in a few different projects:
I feel like supporting https://github.com/bufbuild/protobuf-es natively in the SDK would be a really nice user experience so that clients don't need too many workarounds to enable it; I am aiming for something close to a "it just works" experience Sidenote: I am also admittedly not the most familiar with the node ecosystem's packaging conventions (I primarily write Java and Rust, only using TypeScript where absolutely necessary), so I'm open to whatever direction you prefer, and I'm happy to support this work |
|
I also have a prototype of this working locally with protoc-gen-ts-temporal |
Adds `ProtobufEsBinaryPayloadConverter`, `ProtobufEsJsonPayloadConverter`, and `DefaultPayloadConverterWithProtobufsEs` exported from `@temporalio/common/lib/protobufs-es`, parallel to the existing protobufjs trio at `@temporalio/common/lib/protobufs`. Why --- Downstream codegen tooling (e.g. protoc-gen-ts-temporal) emits `@bufbuild/protobuf` v2 message types, but the SDK only shipped protobufjs-based payload converters. Consumers had to write the bridge themselves to use generated protobuf-es types with Temporal — mechanical work that's easy to get subtly wrong, especially around the proto3 JSON mapping for `google.protobuf.Any`, `oneof`, and `int64`, and that rarely benefits from cross-SDK wire-compatibility review. Bringing a first-class converter into the SDK closes that gap and gives the protobuf-es ecosystem the same out-of-the-box experience the protobufjs path has had since v0. Design ------ * Wire-compatible with the protobufjs converters. Same encoding markers (`binary/protobuf`, `json/protobuf`), same fully-qualified `messageType` metadata, byte-identical binary payloads, and conforming proto3 JSON. A worker on one runtime can decode a payload produced by a client on the other in either binary or JSON form. Composite ordering (Undefined → Binary → ProtobufJson → ProtobufBinary → Json) matches `DefaultPayloadConverterWithProtobufs` and the Go SDK default. * `@bufbuild/protobuf` is declared as an optional peer dependency, so consumers on the protobufjs path pay nothing. The converters live behind a separate `protobufs-es` sub-path entry; workflow bundles that don't import it stay free of the dependency. * Constructor accepts a pre-built `Registry` or a schema array (in which case the converters call `createRegistry` internally) — both forms are common in consumer code. * JSON serialization forwards `this.registry` to `toJson`/`fromJson`, which is required to round-trip `google.protobuf.Any` and proto2 extension fields. * JSON output is semantically equivalent to the protobufjs path but not byte-identical. protobuf-es follows `@bufbuild/protobuf`'s default — and the Go SDK's `protojson.Marshal` default — of omitting fields with implicit (zero) values. protobufjs + `proto3-json-serializer` emits them. Both forms parse correctly by either reader; the wire-bytes difference is documented in `docs/protobuf-libraries.md` and pinned by tests rather than papered over. Tests (42 cases under AVA) -------------------------- * Unit: `toPayload`/`fromPayload` shape, round-trip, error paths (missing or malformed metadata, missing schema, missing registry, bad registry argument), composite-ordering parity with the protobufjs default. * Cross-runtime: binary byte-equality and JSON structural equality between the protobufjs and protobuf-es converters for the same logical message; each runtime decodes the other's output. Covers nested messages, `oneof` (text / number / nested-message branches), `map<string, T>`, and bytes fields. * Edge cases: `int64` (protobuf-es emits the canonical quoted-string form; protobufjs emits a number — divergence asserted explicitly); plain JS objects that happen to share a `$typeName` field surface a clear `PayloadConverterError` rather than silently round-tripping as JSON. * `google.protobuf.Any`: positive round-trip through JSON proves the registry forwarding works; a negative test with an Any-only registry confirms that `toJson` actually consults the registry rather than silently emitting an unrenderable Any. * Sandbox integration (gated on `RUN_INTEGRATION_TESTS`): a workflow round-trips a `BinaryMessage` through the v8 isolate; a second workflow exercises signal / query / update with protobuf-es message types — the surface protoc-gen-ts-temporal generates against — and verifies wire round-trip through Core; a third confirms that a worker whose registry lacks an inbound message type surfaces a clean `PayloadConverterError` as a workflow task failure rather than crashing. Fixtures share `.proto` sources with the protobufjs codegen pipeline under `packages/test/protos/`; `buf` is configured to read the same directory, so the two converters can't drift on what "the same message" means. A `regen:protos-es` npm script and `@bufbuild/buf` as a devDependency mean regeneration works from a clean checkout with no system buf install. Docs ---- `docs/protobuf-libraries.md` gains an "Alternative: @bufbuild/protobuf v2" section alongside the existing protobufjs decision log, with a minimal end-to-end consumer example and the key API-shape differences (`createRegistry` vs protobufjs `Root`, `create(Schema, init)` vs `Message.create`, no `patchProtobufRoot` step, spec-compliant proto3 JSON built in). Includes a direct note on the JSON divergence on implicit defaults with an actionable workaround if byte parity is required.
Drop preferential language and reduce ceremony to a one-paragraph overview plus a minimal payload-converter example.
Replaces the hand-rolled isProtobufEsMessage helper with @bufbuild/protobuf's public isMessage. Tracks future tightening of the brand upstream and removes the MessageShape<DescMessage> casts that were only there because we'd erased the schema type.
ava doesn't guarantee declaration-order execution for non-serial tests, so `Runtime.install` inside the first integration test could race with a Worker constructed by another concurrent test, hitting "runtime already instantiated". Switch the three integration tests to `test.serial` to lock in the ordering the comment was already claiming.
- Hoist the structural-typing ($typeName collision) caveat from a private helper to the public DefaultPayloadConverterWithProtobufsEs JSDoc, where users configuring the converter will actually see it. - Explain why the binary fromPayload rewraps Uint8Array but the JSON path doesn't (TextDecoder accepts cross-realm BufferSource). - Fix a test comment that wrongly claimed $typeName is non-enumerable; it's a regular enumerable assignment in @bufbuild/protobuf.
- Add unit test for `toJson` on google.protobuf.Any with no registry at all (the existing test only covered the "registry has AnySchema but not the inner type" case). - Add cross-runtime test asserting pbjs rejects an es-produced binary payload with `metadata.messageType` stripped — symmetric to the existing es-side malformed-metadata test. - Make the workflow's "finish before input" guard a nonRetryable `ApplicationFailure` instead of a plain `Error` (plain Errors fail the workflow task and retry forever, so the throw was unreachable in practice). Add an integration test that pins this contract.
wcygan
force-pushed
the
feat/protobuf-es-payload-converters
branch
from
48e2e6d to
347ef0d
Compare
Yeah, to be honest, I’d say the existing The main concern is not with Every optional feature we ship as part of the SDK carries long-term maintenance, compatibility, testing, release, and support expectations. Over time, that does not scale very well: it reduces maintainer bandwidth for core SDK work, makes the package surface area harder to reason about, and blurs the line between “core Temporal SDK behavior” and “integration code for a particular ecosystem/toolchain.” It also makes us more cautious about future changes, because users reasonably expect anything shipped in the SDK itself to follow the SDK’s reliability and compatibility standards. We’re trying to move toward a repo/ecosystem model that is more scalable for us and more open to community contributions. In particular, we’ve been extracting optional features either into the contrib area or into external community-maintained repositories, depending on the case. We’re also encouraging integrations like this to use the newly introduced Plugin API, which should significantly improve the DX for users of optional features without requiring those features to live directly in the core SDK package. For external community-maintained integrations, we’ve started the Temporal Code Exchange initiative to give those contributions better visibility, so users can still discover them without everything needing to live directly in the SDK repo. So, what about this specific pull request? I think that, at this time, an external community-maintained package may be the better fit than adding this to our The main reason is that this converter seems closely tied to the protoc codegen plugin you’re building. In practice, the interesting evolution is likely to happen at that integration layer: what the generated types look like, how schemas are registered, how cross-language payload compatibility is exposed, what conventions the generator establishes, etc. By contrast, the Temporal payload converter API itself is relatively stable. If we put only the converter piece in the SDK repo, but the generator and surrounding workflow live elsewhere, ownership gets split in a way that would be inefficient for both teams. Changes to the end-to-end experience would need coordination across two repositories, and in some cases you’d need SDK maintainer involvement for changes that are really driven by the generator’s design. So my intuition, at least for now, is that the best ownership model is for the If you’re open to that direction, I’d be happy to help figure out what the right contrib/plugin shape should look like. |
2 participants
Resolves #2045.
I would like to use this in my project protoc-gen-ts-temporal which is similar to protoc-gen-go-temporal from "Go Code Generation with Temporal & Protobufs"
This would be useful to me because I could run an HTTP server in TypeScript and use a typed client (this PR + protoc-gen-ts-temporal) to begin a workflow that executes on a Rust worker
Summary
Adds these in
@temporalio/common/lib/protobufs-es:ProtobufEsBinaryPayloadConverterProtobufEsJsonPayloadConverterDefaultPayloadConverterWithProtobufsEsWire-compatible with the protobufjs path — binary bytes are byte-identical; JSON is canonical proto3.