proto2 required support

[zeroslope]

The required syntax of proto2 is now optional in the generated type, is there any way to make it required?

syntax = "proto2";

message Foo {
  required string a = 1;
  required string b = 2;
}

// @generated by protoc-gen-es v1.0.0 with parameter "target=ts"
// @generated from file foo.proto (syntax proto2)
/* eslint-disable */
// @ts-nocheck

import type { BinaryReadOptions, FieldList, JsonReadOptions, JsonValue, PartialMessage, PlainMessage } from "@bufbuild/protobuf";
import { Message, proto2 } from "@bufbuild/protobuf";

/**
 * @generated from message Foo
 */
export class Foo extends Message<Foo> {
  /**
   * @generated from field: required string a = 1;
   */
  a?: string;

  /**
   * @generated from field: required string b = 2;
   */
  b?: string;

  constructor(data?: PartialMessage<Foo>) {
    super();
    proto2.util.initPartial(data, this);
  }

  static readonly runtime = proto2;
  static readonly typeName = "Foo";
  static readonly fields: FieldList = proto2.util.newFieldList(() => [
    { no: 1, name: "a", kind: "scalar", T: 9 /* ScalarType.STRING */ },
    { no: 2, name: "b", kind: "scalar", T: 9 /* ScalarType.STRING */ },
  ]);

  static fromBinary(bytes: Uint8Array, options?: Partial<BinaryReadOptions>): Foo {
    return new Foo().fromBinary(bytes, options);
  }

  static fromJson(jsonValue: JsonValue, options?: Partial<JsonReadOptions>): Foo {
    return new Foo().fromJson(jsonValue, options);
  }

  static fromJsonString(jsonString: string, options?: Partial<JsonReadOptions>): Foo {
    return new Foo().fromJsonString(jsonString, options);
  }

  static equals(a: Foo | PlainMessage<Foo> | undefined, b: Foo | PlainMessage<Foo> | undefined): boolean {
    return proto2.util.equals(Foo, a, b);
  }
}

[timostamm]

We agree that this would be nice for type-safety, but it means we have to make properties mandatory in the constructor, something we would prefer to avoid. We haven't found the best way forward yet for this conundrum, so for the time being, proto2 required properties are typed as optional, but will raise an error at runtime when serializing, following the pattern of other implementations.

Thank you for raising this issue though. It is very possible that we'll be making some changes for better support in of proto2 required in the not-so-distant future, so we're happy to keep this open.

[nicolasalt]

What are the cons of mandatory properties in the constructor or using a builder pattern?

On the other hand, there are clear benefits of not having to check fields for "undefined" that are supposed to be required in the schema.

[joemckenney]

The behavior has changed since this issue was filed. The generated classes for proto2 messages seem to reflect the required/optional fields. See below.

That being said, some imported messages do not seem to generate correctly (e.g. Timestamp is required in the message but optional in the class).

Proto Message (sample)

syntax = "proto2";

package event_logs.v1;

import "google/protobuf/timestamp.proto";

enum FlowStateChange {
  FLOW_STATE_CHANGE_UNSPECIFIED = 0;
  FLOW_STATE_CHANGE_STARTED = 1;
  FLOW_STATE_CHANGE_STOPPED = 2;
  FLOW_STATE_CHANGE_FINISHED = 3;
  FLOW_STATE_CHANGE_RESET = 4;
}

message FlowStateChangeEvent {
  required uint32 workspace = 1;
  required uint32 environment = 2;
  optional uint32 group = 3;
  required uint32 user = 4;
  required uint32 flow = 5;
  required uint32 version = 6;
  required FlowStateChange state = 7;
  required bool value = 8;
  required google.protobuf.Timestamp timestamp = 9;
}

Generated Class

/**
 * @generated from message event_logs.v1.FlowStateChangeEvent
 */
export declare class FlowStateChangeEvent extends Message<FlowStateChangeEvent> {
  /**
   * @generated from field: required uint32 workspace = 1;
   */
  workspace: number;

  /**
   * @generated from field: required uint32 environment = 2;
   */
  environment: number;

  /**
   * @generated from field: optional uint32 group = 3;
   */
  group?: number;

  /**
   * @generated from field: required uint32 user = 4;
   */
  user: number;

  /**
   * @generated from field: required uint32 flow = 5;
   */
  flow: number;

  /**
   * @generated from field: required uint32 version = 6;
   */
  version: number;

  /**
   * @generated from field: required event_logs.v1.FlowStateChange state = 7;
   */
  state: FlowStateChange;

  /**
   * @generated from field: required bool value = 8;
   */
  value: boolean;

  /**
   * @generated from field: required google.protobuf.Timestamp timestamp = 9;
   */
  timestamp?: Timestamp;

  constructor(data?: PartialMessage<FlowStateChangeEvent>);

  static readonly runtime: typeof proto2;
  static readonly typeName = "event_logs.v1.FlowStateChangeEvent";
  static readonly fields: FieldList;

  static fromBinary(bytes: Uint8Array, options?: Partial<BinaryReadOptions>): FlowStateChangeEvent;

  static fromJson(jsonValue: JsonValue, options?: Partial<JsonReadOptions>): FlowStateChangeEvent;

  static fromJsonString(jsonString: string, options?: Partial<JsonReadOptions>): FlowStateChangeEvent;

  static equals(a: FlowStateChangeEvent | PlainMessage<FlowStateChangeEvent> | undefined, b: FlowStateChangeEvent | PlainMessage<FlowStateChangeEvent> | undefined): boolean;
}

[timostamm]

The behavior has changed since this issue was filed. The generated classes for proto2 messages seem to reflect the required/optional fields. See below.

@joemckenney, this was actually a bug 😱, fixed in v1.7.2.

To support editions, we're working on better support for field presence, and plan to update the logic for proto2 first. I'll keep this issue updated.

[volgar1x]

@timostamm @joemckenney I cannot reproduce the generated class FlowStateChangeEvent with "@bufbuild/protoc-gen-es": "^1.7.2".

Here are the commands I use :

SED := `which gsed 2>/dev/null || which sed`
TS_SRC_DIR := "src"
TS_DIST_DIR := "dist"

default:
    just --list

build: _init_dist _build-es-pb _build-es-connect
    tsc -b

clean:
    rm -rf dist

_init_dist:
    mkdir -p "{{TS_DIST_DIR}}"

_build-es-pb target="showhost.proto":
    protoc -I . \
        --es_out "{{TS_DIST_DIR}}" \
        --es_opt target=js+dts \
        --es_opt import_extension=none \
        "{{target}}"

_build-es-connect target="showhost.proto":
    protoc -I . \
        --connect-es_out "{{TS_DIST_DIR}}" \
        --connect-es_opt target=js+dts \
        --connect-es_opt import_extension=none \
        "{{target}}"

Here is my package.json :

{
    "private": true,
    "name": "@showhost/transport",
    "main": "src/index.ts",
    "types": "src/index.ts",
    "version": "0.1.0",
    "scripts": {
        "build": "just build",
        "clean": "just clean"
    },
    "files": [
        "dist"
    ],
    "exports": {
        ".": "./dist/index.js",
        "./*": "./dist/*.js"
    },
    "devDependencies": {
        "@bufbuild/protoc-gen-es": "^1.7.2",
        "@connectrpc/protoc-gen-connect-es": "^1.3.0"
    },
    "dependencies": {
        "@bufbuild/protobuf": "^1.7.2",
        "@connectrpc/connect": "^1.3.0",
        "@connectrpc/connect-web": "^1.3.0"
    }
}

And here is the (partial) output I get :

/**
 * @generated from message xyz.volgar1x.showhost.FlowStateChangeEvent
 */
export declare class FlowStateChangeEvent extends Message<FlowStateChangeEvent> {
  /**
   * @generated from field: required uint32 workspace = 1;
   */
  workspace?: number;

  /**
   * @generated from field: required uint32 environment = 2;
   */
  environment?: number;

  /**
   * @generated from field: optional uint32 group = 3;
   */
  group?: number;

  /**
   * @generated from field: required uint32 user = 4;
   */
  user?: number;

  /**
   * @generated from field: required uint32 flow = 5;
   */
  flow?: number;

  /**
   * @generated from field: required uint32 version = 6;
   */
  version?: number;

  /**
   * @generated from field: required xyz.volgar1x.showhost.FlowStateChange state = 7;
   */
  state?: FlowStateChange;

  /**
   * @generated from field: required bool value = 8;
   */
  value?: boolean;

  /**
   * @generated from field: required google.protobuf.Timestamp timestamp = 9;
   */
  timestamp?: Timestamp;

  constructor(data?: PartialMessage<FlowStateChangeEvent>);

  static readonly runtime: typeof proto2;
  static readonly typeName = "xyz.volgar1x.showhost.FlowStateChangeEvent";
  static readonly fields: FieldList;

  static fromBinary(bytes: Uint8Array, options?: Partial<BinaryReadOptions>): FlowStateChangeEvent;

  static fromJson(jsonValue: JsonValue, options?: Partial<JsonReadOptions>): FlowStateChangeEvent;

  static fromJsonString(jsonString: string, options?: Partial<JsonReadOptions>): FlowStateChangeEvent;

  static equals(a: FlowStateChangeEvent | PlainMessage<FlowStateChangeEvent> | undefined, b: FlowStateChangeEvent | PlainMessage<FlowStateChangeEvent> | undefined): boolean;
}

Do you have the right command invocation notation?

Thanks for your work anyway! protobuf-es is a key component of my project! 🙂

[anotherchudov]

Hello,

We would like to generate types from protobuf definition that would look like this:

export type RequiredObject = {
  absolutelyRequired: string;
};

export type Request = {
  obj: RequiredObject;
  something?: [],
};

We have tried this:

syntax = "proto2";
package some_package;

message RequiredObject {
    required string absolutely_required = 1;
}
message OptionalArray {
    repeated string values = 1;
}

message Request {
    required RequiredObject obj = 1;
    optional OptionalArray something = 2;
}

and based on description of v2 alpha release #801 expected to get something like

export type RequiredObject = {
  absolutelyRequired: string;
};

export type OptionalArray = {
  values: string[];
};

export type Request = {
  obj: RequiredObject;
  something?: OptionalArray,
};

but in generated type RequiredObject obj is still optional.

Is this in the works?

[timostamm]

Apologies for the confusion. I've updated the v2 release notes to clarify that it's an improvement over v1. Required scalar and enum fields are no longer optional in generated code, but this does not apply to message fields. This would need changes that impact the entire API, see #532 (comment).

I've been working with JSON and TypeScript for a long time, and I understand the desire to have non-optional properties. Realistically though, the required keyword is not the right solution. It's a legacy feature, and the official language guide states: Do not use. It's also not available in proto3, and while it's available in edition 2023, it's likely to be removed in a future edition.

From my understanding, the important bit is avoiding the nullish-coalescing operator and checking for undefined for cases where the API guarantees that a message is present (also see #532). This can potentially be solved by an implementation of protovalidate for TypeScript that ties into the type system. We can't make promises yet, but it looks promising.

[nicolasalt]

The Google's recomendation is relevant for cases where the developer doesn't fully control both sides the use the same proto message, for example, one side is a phone app that can't be forcibly updated.
However, there are plenty of other use-cases where the developer has full control, for example, when using web workers, Electron processes, etc. For these cases a fast, built-in, fitting into the Typescript semantics "required" validation could be better than using a "protovalidate" or other post-processing solution, that would add complexity and latency.

[timostamm]

Certainly. But would you want to be stuck with a no longer maintained proto2 down the road? Do you think that switching to getters or immutable is the right call for general use?

Protobuf-ES v2 is just descriptors and functions. For specific use cases like private IPC, you can easily wrap fromBinary and return a custom generated or even just a mapped type, based on a simple custom option.

[nicolasalt]

I imagine for the private IPC use-case people (who like protobufs) are evaluting different libraries for various criteria, including the amount of extra work and maintenance they have to do. From this perspective having to do a separate codegen themselves seems to be a big drawback, it would be simpler to pick another library.

on the proto2 vs. proto3 topic: In my understanding various Typescript proto2 libraries are still well maintained, including protobuf-es. The proto2 spec has been battle-tested for years, not sure if it requires any maintenance. I personally think that proto2 schema is better than proto3, in particular around "required" and "has" semantics.

For #532 (comment), I don't have enough data to have a strong opinion. Naively I'd say that enforcing immutability would be too much of a burden for the JS/TS users and they would switch to a different library. As a data point, in the official Java codegen, if I remember correctly, (for builders, which are essentially mutable messages) they create an mutable object and cache it inside. This mostly worked but created unpredictable memory and performance patterns, which were noticeable in some rare cases.

[lukasIO]

would love to see better required support as well.

But would you want to be stuck with a no longer maintained proto2 down the road

Google opted to support proto2 "pretty much forever" due to their own dependency internally on proto2 specific features.

[timostamm]

And yet, Google also has plans to "deprecate/remove required field presence": protocolbuffers/protobuf@0b6e768

[lukasIO]

sure, in editions, right?
This doesn't contradict proto2 syntax being supported for the foreseeable future though.

[timostamm]

I'm not sure. We plan to add *Valid types, similar to JSON types, which will honor protovalidate standard constraints such as (buf.validate.field).required = true. Tracked in #966. This implements required semantics at a different level, as suggested here in the language guide.

It should be possible to use these types for construction and RPC in a flexible way, maybe also with a no-op validator if you trust the source. And I don't see why we couldn't also honor proto2 required and the LEGACY_REQUIRED feature.

[lukasIO]

cool! that sounds like it would do the trick :)