Return proxy from constructor further up the prototype chain

Borrowed from eb142d8 that was supposed to land in the now-closed tj#1921. The change ensures the this object used in Command is the proxy from the very beginning, even in the constructor. This solves pretty much all issues with returning the proxy from a constructor. For example, private fields can be used now. More details available at: tj#1921 (comment) Additionally, the ownKeys() trap and wrong spelling in comments have been fixed (the former change being borrowed from fc927c8).
aweebit · Aug 3, 2023 · b53703b · b53703b
1 parent be00f59
commit b53703b
Showing 1 changed file with 71 additions and 62 deletions.
diff --git a/lib/command.js b/lib/command.js
@@ -12,7 +12,71 @@ const { suggestSimilar } = require('./suggestSimilar');
 
 // @ts-check
 
-class Command extends EventEmitter {
+class CommandBase extends EventEmitter {
+  constructor() {
+    super();
+
+    // The proxy only treats keys not present in the instance and its prototype chain as keys for _optionValues when _storeOptionsAsProperties is set to true.
+    // Setting option values for keys present in the instance and its prototype chain is still possible by calling .setOptionValue() or .setOptionValueWithSource(),
+    // but such values will not be accessible as instance properties because the instance and its prototype chain have precedence.
+    // However, they will be accessible via .getOptionValue(), .opts() and .optsWithGlobals().
+    return new Proxy(this, {
+      get(target, key, receiver) {
+        if (target._storeOptionsAsProperties && !(key in target)) {
+          target = receiver = receiver._optionValues;
+        }
+        return Reflect.get(target, key, receiver);
+      },
+      set(target, key, value, receiver) {
+        if (target._storeOptionsAsProperties && !(key in target)) {
+          target = receiver = receiver._optionValues;
+        }
+        return Reflect.set(target, key, value, receiver);
+      },
+      has(target, key) {
+        if (target._storeOptionsAsProperties && !(key in target)) {
+          target = target._optionValues;
+        }
+        return Reflect.has(target, key);
+      },
+      deleteProperty(target, key) {
+        if (target._storeOptionsAsProperties && !(key in target)) {
+          target = target._optionValues;
+        }
+        return Reflect.deleteProperty(target, key);
+      },
+      defineProperty(target, key, descriptor) {
+        if (target._storeOptionsAsProperties && !(key in target)) {
+          target = target._optionValues;
+        }
+        return Reflect.defineProperty(target, key, descriptor);
+      },
+      getOwnPropertyDescriptor(target, key) {
+        if (target._storeOptionsAsProperties && !(key in target)) {
+          target = target._optionValues;
+        }
+        return Reflect.getOwnPropertyDescriptor(target, key);
+      },
+      ownKeys(target) {
+        const result = Reflect.ownKeys(target);
+        if (target._storeOptionsAsProperties) {
+          result.push(...Reflect.ownKeys(target._optionValues).filter(
+            key => !(result.includes(key)) // remove duplicates
+          ));
+        }
+        return result;
+      },
+      preventExtensions(target) {
+        if (target._storeOptionsAsProperties) {
+          Reflect.preventExtensions(target._optionValues);
+        }
+        return Reflect.preventExtensions(target);
+      }
+    });
+  }
+}
+
+class Command extends CommandBase {
   /**
    * Initialize a new `Command`.
    *
@@ -70,74 +134,19 @@ class Command extends EventEmitter {
     this._helpCommandDescription = 'display help for command';
     this._helpConfiguration = {};
 
+    // Because of how the proxy returned from the CommandBase constructor works in order to support options-as-properties,
+    // all instance properties have to be defined when _storeOptionsAsProperties is set to false.
+    // Ideally, that should happen as soon as in the constructor, even if it seems unnecessary because the initial values are undefined like here.
+    this._version = undefined;
+    this._versionOptionName = undefined;
+
     this._unprocessedName = name || '';
     this._persistentOptionValues = {};
     this._persistentOptionValueSources = {};
     this.resetParseState();
 
     /** @type {boolean | undefined} */
     this._asyncParsing = undefined;
-
-    // Because of how the returned proxy works, ideally, no prooerties should be defined outside the cinstructor.
-    // They can still be defined outside the constructor in subclasses, but only when _storeOptionsAsProperties is set to false.
-    this._version = undefined;
-    this._versionOptionName = undefined;
-
-    // The proxy only treats keys not present in the instance and its prototype chain as keys for _optionValues when _storeOptionsAsProperties is set to true.
-    // Setting option values for keys present in the instance and its prototype chain is still possible by calling .setOptionValue() or .setOptionValueWithSource(),
-    // but such values will not be accessible as instnace properties because the instance and its prototype chain has precedence.
-    // However, they will be accessible via .getOptionValue(), .opts() and .optsWithGlobals().
-    return new Proxy(this, {
-      get(target, key, receiver) {
-        if (target._storeOptionsAsProperties && !(key in target)) {
-          target = receiver = receiver._optionValues;
-        }
-        return Reflect.get(target, key, receiver);
-      },
-      set(target, key, value, receiver) {
-        if (target._storeOptionsAsProperties && !(key in target)) {
-          target = receiver = receiver._optionValues;
-        }
-        return Reflect.set(target, key, value, receiver);
-      },
-      has(target, key) {
-        if (target._storeOptionsAsProperties && !(key in target)) {
-          target = target._optionValues;
-        }
-        return Reflect.has(target, key);
-      },
-      deleteProperty(target, key) {
-        if (target._storeOptionsAsProperties && !(key in target)) {
-          target = target._optionValues;
-        }
-        return Reflect.deleteProperty(target, key);
-      },
-      defineProperty(target, key, descriptor) {
-        if (target._storeOptionsAsProperties && !(key in target)) {
-          target = target._optionValues;
-        }
-        return Reflect.defineProperty(target, key, descriptor);
-      },
-      getOwnPropertyDescriptor(target, key) {
-        if (target._storeOptionsAsProperties && !(key in target)) {
-          target = target._optionValues;
-        }
-        return Reflect.getOwnPropertyDescriptor(target, key);
-      },
-      ownKeys(target) {
-        const result = Reflect.ownKeys(target);
-        if (target._storeOptionsAsProperties) {
-          result.push(...Reflect.ownKeys(target._optionValues));
-        }
-        return result;
-      },
-      preventExtensions(target) {
-        if (target._storeOptionsAsProperties) {
-          Reflect.preventExtensions(target._optionValues);
-        }
-        return Reflect.preventExtensions(target);
-      }
-    });
   }
 
   resetParseState() {