New implementation that explicitly respects the optional keyword

Author: martin-traverse

cli/README.md

@@ -68,8 +68,7 @@ Translates between file formats and generates static code.
   --force-long     Enforces the use of 'Long' for s-/u-/int64 and s-/fixed64 fields.
   --force-number   Enforces the use of 'number' for s-/u-/int64 and s-/fixed64 fields.
   --force-message  Enforces the use of message instances instead of plain objects.
-  --null-defaults  Default value for optional fields is null instead of zero value.
-  --pb3-optional   Make type declarations respect optional fields for PB3.
+  --force-optional Enforces nullable types for fields marked as optional (proto2 and proto3)
 
 usage: pbjs [options] file1.proto file2.json ...  (or pipe)  other | pbjs [options] -
 ```

cli/pbjs.js

@@ -62,8 +62,8 @@ exports.main = function main(args, callback) {
             "force-number": false,
             "force-enum-string": false,
             "force-message": false,
-            "null-defaults": false,
-            "pb3-optional": false
+            "force-optional": false,
+            "null-defaults": false
         }
     });
 
@@ -147,9 +147,9 @@ exports.main = function main(args, callback) {
                 "  --force-long     Enforces the use of 'Long' for s-/u-/int64 and s-/fixed64 fields.",
                 "  --force-number   Enforces the use of 'number' for s-/u-/int64 and s-/fixed64 fields.",
                 "  --force-message  Enforces the use of message instances instead of plain objects.",
+                "  --force-optional Enforces nullable types for fields marked as optional (proto2 and proto3)",
                 "",
-                "  --null-defaults  Default value for optional fields is null instead of zero value.",
-                "  --pb3-optional   Make type declarations respect optional fields for PB3.",
+                "  --null-defaults  Default value for optional fields is null instead of zero value (deprecated, use --force-optional instead)",
                 "",
                 "usage: " + chalk.bold.green("pbjs") + " [options] file1.proto file2.json ..." + chalk.gray("  (or pipe)  ") + "other | " + chalk.bold.green("pbjs") + " [options] -",
                 ""

cli/targets/static.js

@@ -354,14 +354,6 @@ function toJsType(field) {
     return type;
 }
 
-function handleOptionalFields(jsType, field) {
-
-    if (field.optional && !field.map && !field.repeated && field.resolvedType instanceof Type || field.partOf)
-        return jsType + "|null|undefined";
-    else
-        return jsType
-}
-
 function buildType(ref, type) {
 
     if (config.comments) {
@@ -375,10 +367,13 @@ function buildType(ref, type) {
             prop = prop.substring(1, prop.charAt(0) === "[" ? prop.length - 1 : prop.length);
             var jsType = toJsType(field);
 
-            // Hide the fix for PB3 optional fields behind a config flag, it is an API change in generated output
-            if (config.pb3Optional) {
-                jsType = handleOptionalFields(jsType, field);
+            // New behaviour - fields explicitly marked as optional and members of a one-of are nullable
+            // Maps and repeated fields are not explicitly optional, they default to empty instances
+            if (config["force-optional"]) {
+                if (field.explicitOptional || field.partOf)
+                    jsType = jsType + "|null|undefined";
             }
+            // Old behaviour - field.optional is true for all fields in proto3
             else {
                 if (field.optional)
                     jsType = jsType + "|null";
@@ -411,10 +406,13 @@ function buildType(ref, type) {
             push("");
             var jsType = toJsType(field);
 
-            // Hide the fix for PB3 optional fields behind a config flag, it is an API change in generated output
-            if (config.pb3Optional) {
-                jsType = handleOptionalFields(jsType, field);
+            // New behaviour - fields explicitly marked as optional and members of a one-of are nullable
+            // Maps and repeated fields are not explicitly optional, they default to empty instances
+            if (config["force-optional"]) {
+                if (field.explicitOptional || field.partOf)
+                    jsType = jsType + "|null|undefined";
             }
+            // Old behaviour - field.optional is true for all fields in proto3
             else {
                 if (field.optional && !field.map && !field.repeated && (field.resolvedType instanceof Type || config["null-defaults"]) || field.partOf)
                     jsType = jsType + "|null|undefined";
@@ -430,11 +428,16 @@ function buildType(ref, type) {
             push("");
             firstField = false;
         }
+        // New behaviour sets a null default when the optional keyword is used explicitly
+        // Old behaviour considers all proto3 fields optional and uses the null-defaults config flag
+        var nullDefault = config["force-optional"]
+            ? field.explicitOptional
+            : field.optional && config["null-defaults"];
         if (field.repeated)
             push(escapeName(type.name) + ".prototype" + prop + " = $util.emptyArray;"); // overwritten in constructor
         else if (field.map)
             push(escapeName(type.name) + ".prototype" + prop + " = $util.emptyObject;"); // overwritten in constructor
-        else if (field.partOf || field.optional && config["null-defaults"])
+        else if (field.partOf || nullDefault)
             push(escapeName(type.name) + ".prototype" + prop + " = null;"); // do not set default value for oneof members
         else if (field.long)
             push(escapeName(type.name) + ".prototype" + prop + " = $util.Long ? $util.Long.fromBits("

src/field.js

@@ -117,6 +117,14 @@ function Field(name, id, type, rule, extend, options, comment) {
      */
     this.optional = !this.required;
 
+    /**
+     * Whether this field is explicitly marked as optional
+     * Proto3 fields must use the optional keyword
+     * Maps and repeated fields are not explicitly optional
+     * @type {boolean}
+     */
+    this.explicitOptional = rule === "optional";
+
     /**
      * Whether this field is repeated.
      * @type {boolean}