Unexpected null behaviour (TS, --force-message, properties and members)

[martin-traverse]

protobuf.js version: 7.3.2

I believe there is an issue with the way null is handled in the JSDoc / TS type definitions. Specifically, the construction of properties / members in JSDoc (and equivalent interfaces / messages in TS definitions). The --force-message flag affects this but doesn't behave as I would expect / like.

With --force-message=true, interfaces get built with messages. So when you call .create() or new MessageType(), the top level type is an interface, but any sub-types are declared as messages which makes declaring them in code overly verbose. OTOH, with --force-message=false messages get built with interfaces, so for message returned from API calls (and messages already built using create() in code) almost everything appears nullable - particularly repeated fields and map types, which are never null after the message has been built. So whichever way you go there is an issue with nullability, either when you are building objects or when you are using them.

A more natural approach to my mind would be that interfaces are built using other interfaces, and messages are built using other messages. Then the --force-message flag only affects what is needed in the method calls, not the construction of the types themselves.

I've made a PR for this change - it's fairly small / simple. If you agree this would be a helpful change, the question is how to release it:

a) Treat it as a fix and a breaking change, so make a suitable version bump.
b) Treat it as a fix but put it behind a feature flag, e.g. --null-compat, with the intention to eventually remove the old behaviour
c) Treat is as a feature, put it behind a feature flag and preserve both options indefinitely

This also relates to the proto3 optional semantics (see issue #1779, PR #2011), there could end up being a lot of flags controlling this behaviour which might be confusing and lead to unexpected affects as code is maintained over time. With that in mind, my vote would be for options (a) or (b). With option (b) the fix for proto3 optional support could go behind the same flag.

Examples

Here is an excerpt from our metadata definitions:

syntax = "proto3"

enum SchemaType { ... };
enum PartType { ... };

message SchemaDefinition {
   SchemaType schemaType = 1;
   PartType partType = 2;
   oneof schemaTypeDefinition {
      TableSchema table = 3;
      ...
   }
}

message TableSchema {
   repeated FieldSchema fields = 1;
}

message FieldSchema {
   string fieldName = 1;
   ...
   optional string formatCode = 7;
}

To create one of these definitions to send to our API, we'd use something like this:

const fields = fieldInfo.map(f => { ... });

const schema = tracdap.metadata.SchemaDefinition.create({
   schemaType: tracdap.SchemaType.TABLE,
   table: {
      fields: fields
   }
})

With --force-message=true, this reports a whole bunch of warnings (and in TS these are upgraded to errors). But then when we get the object back:

serviceApi.getObject(schemaId).then(response => {

   const schema = response.definition.schema;
   schema.table.fields.forEach(f => {
      // Do something with the field
   });
});

Now, with --force-message=false so many of these things become nullable, and so that code needs a lot of extra null checks.

By building interfaces with interfaces, and messages with messages, the objects used to create messages become much more forgiving, and the responses don't report fields as nullable when they are not.

Please let me know if this change would be useful, happy to do any work to add flags etc. if required.

[martin-traverse]

Associate PR is #2013

[martin-traverse]

This issue is now covered by PR #2011