chore(proto)!: bump protobufjs to v8 and protobufjs-cli to v2 - #2303
chore(proto)!: bump protobufjs to v8 and protobufjs-cli to v2#2303mjameswh wants to merge 6 commits into
Conversation
…decode too The breaking change entry framed the omission of default-valued fields as a ProtoJSON concern, but protobufjs 8 also stops materializing them when decoding binary payloads and in fromObject. What changes is own-property enumeration — Object.keys, spreads and deep equality — rather than reading the field, which still yields the default.
The features suite on main still uses proto3-json-serializer and pins protobufjs 7, which is incompatible with this change. Point the job at temporalio/features#863 until that PR merges, then revert this.
| - The `getTypeUrl` method on generated message types is no longer declared, though it remains | ||
| callable at runtime. |
There was a problem hiding this comment.
Do we want to advertise that it is still callable? If it is no longer declared could it be removed upstream without it being considered a breaking change?
| const systemInfo = await temporalnexus | ||
| .getClient() | ||
| .connection.workflowService.getSystemInfo({ namespace: 'default' }); | ||
| const systemInfo = await temporalnexus.getClient().connection.workflowService.getSystemInfo({}); |
There was a problem hiding this comment.
Is this releated to the protobuf bump?
There was a problem hiding this comment.
It's a latent bug that the protobufjs bump exposed.
There's actually no namespace property on IGetSystemInfoRequest, so the former was technically incorrect, but TSC was not reporting this as an error given the previous definition of that type. It is now, so I had to remove the extraneous property.
| ContinueAsNewInitiator.CONTINUE_AS_NEW_INITIATOR_UNSPECIFIED | ||
| ); | ||
| const initiator = hist.events?.[0].workflowExecutionStartedEventAttributes?.initiator; | ||
| t.true(initiator === undefined || initiator === ContinueAsNewInitiator.CONTINUE_AS_NEW_INITIATOR_UNSPECIFIED); |
There was a problem hiding this comment.
What are the situations where it is undefined vs being a set value? Does the assertion need to handle both?
There was a problem hiding this comment.
With protobufjs v8, fields no longer fills default values for unset fields. That's in line with how protobuf works in other language.
In this case, CONTINUE_AS_NEW_INITIATOR_UNSPECIFIED is the default value for the initiator. So in v7, protobufjs guaranteed that initiator would be CONTINUE_AS_NEW_INITIATOR_UNSPECIFIED if not explicitly set in the serialized message. In v8, it would be undefined in that case. But it could be CONTINUE_AS_NEW_INITIATOR_UNSPECIFIED if the value was specifically set in the message.
2 participants
Summary
Upgrades
protobufjs7.6.4 -> 8.7.1 andprotobufjs-cli1.x -> 2.6.1.This major version bump resolves multiple issues we had previously, allowing cleaning up various workarounds from our code base:
protobufjs. The parser bug has been fixed upstream (fix: Parse aggregate objects in option arrays protobufjs/protobuf.js#2379).proto3-json-serializeris replaced byprotobufjs/ext/protojson.proto3-json-serializerhas noprotobufjs8 support and no prospect of it.protobufjs's extension is spec-compliant out of the box, removing the need forfixPayloads,fixBuffers, and theBufferglobal shimming inside the sandbox.replaceBuffershad to stay, but is now skipped outright ifBufferis absent (e.g. in the Workflow sandbox).fsshim forprotobufjsis gone, since v8'ssrc/util/fs.jsusesrequire(/* webpackIgnore: true */ "fs"). That comment only takes effect withmodule.parser.javascript.commonjsMagicComments, which is now enabled in the bundler.protobufjsnow properly defines its dependency tolong, so injection of that dependency (inprotos/root.js) goes away.This has some visible consequences on generated protobuf types, which are exposed as part of
@temporalio/proto. See details inCHANGELOG.md.