From e9b885642c2b5cd430927e34cee46db31a76efdc Mon Sep 17 00:00:00 2001 From: aweebit <36817090+aweebit@users.noreply.github.com> Date: Sun, 30 Jul 2023 07:23:49 +0300 Subject: [PATCH 1/5] Prevent opts() from exposing private object --- lib/command.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/command.js b/lib/command.js index 590a271dd..2ad575af3 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1567,7 +1567,7 @@ Expecting one of '${allowedValues.join("', '")}'`); return result; } - return this._optionValues; + return { ...this._optionValues }; } /** From 3cfd9b129ec71731af187ba4521b0fecd72b15ef Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Sun, 30 Jul 2023 16:39:11 +0300 Subject: [PATCH 2/5] Make opts() return proxy preventing mutations A proxy is returned only when options-as-properties are disabled. --- lib/command.js | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/lib/command.js b/lib/command.js index 2ad575af3..f7e7a2ca2 100644 --- a/lib/command.js +++ b/lib/command.js @@ -77,6 +77,24 @@ class Command extends EventEmitter { this._helpCommandnameAndArgs = 'help [command]'; this._helpCommandDescription = 'display help for command'; this._helpConfiguration = {}; + + this._optionValuesProxy = new Proxy(this._optionValues, { + set(_, key) { + throw new Error(`Tried to set value of option ${key} directly. +Use .setOptionValue() or .setOptionValueWithSource() instead`); + }, + deleteProperty(_, key) { + throw new Error(`Tried to delete value of option ${key}. +Option value deletion is not supported`); + }, + defineProperty(_, key) { + throw new Error(`Tried to configure value of option ${key} using +- Object.defineProperty(), +- or Object.defineProperties(), +- or Reflect.defineProperty(). +Options value configuration is not supported`); + } + }); } /** @@ -1567,7 +1585,7 @@ Expecting one of '${allowedValues.join("', '")}'`); return result; } - return { ...this._optionValues }; + return this._optionValuesProxy; } /** From 88d0cf846b95bf5b7eb63bd5fd9bd04bb00bf7aa Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Sun, 30 Jul 2023 18:58:01 +0300 Subject: [PATCH 3/5] Allow setting properties directly on opts() return value --- lib/command.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/command.js b/lib/command.js index f7e7a2ca2..c19197787 100644 --- a/lib/command.js +++ b/lib/command.js @@ -79,15 +79,15 @@ class Command extends EventEmitter { this._helpConfiguration = {}; this._optionValuesProxy = new Proxy(this._optionValues, { - set(_, key) { - throw new Error(`Tried to set value of option ${key} directly. -Use .setOptionValue() or .setOptionValueWithSource() instead`); + set: (_, key, value) => { + this.setOptionValue(key, value); + return true; }, - deleteProperty(_, key) { + deleteProperty: (_, key) => { throw new Error(`Tried to delete value of option ${key}. Option value deletion is not supported`); }, - defineProperty(_, key) { + defineProperty: (_, key) => { throw new Error(`Tried to configure value of option ${key} using - Object.defineProperty(), - or Object.defineProperties(), From 1b94efb5472642f6cffd1225aa6cf11864a902eb Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Sun, 30 Jul 2023 23:47:15 +0300 Subject: [PATCH 4/5] Return proxy redirecting keys for options-as-properties from Command constructor Borrowed from #1919. Makes _optionValues the only true storage for option values. Has the added benefit of supporting option names conflicting with instance's properties even when options-as-properties are enabled. --- lib/command.js | 74 ++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 65 insertions(+), 9 deletions(-) diff --git a/lib/command.js b/lib/command.js index c19197787..636c06172 100644 --- a/lib/command.js +++ b/lib/command.js @@ -95,6 +95,67 @@ Option value deletion is not supported`); Options value configuration is not supported`); } }); + + // 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._optionValuesProxy; + } + return Reflect.get(target, key, receiver); + }, + set(target, key, value, receiver) { + if (target._storeOptionsAsProperties && !(key in target)) { + target = receiver = receiver._optionValuesProxy; + } + return Reflect.set(target, key, value, receiver); + }, + has(target, key) { + if (target._storeOptionsAsProperties && !(key in target)) { + target = target._optionValuesProxy; + } + return Reflect.has(target, key); + }, + deleteProperty(target, key) { + if (target._storeOptionsAsProperties && !(key in target)) { + target = target._optionValuesProxy; + } + return Reflect.deleteProperty(target, key); + }, + defineProperty(target, key, descriptor) { + if (target._storeOptionsAsProperties && !(key in target)) { + target = target._optionValuesProxy; + } + return Reflect.defineProperty(target, key, descriptor); + }, + getOwnPropertyDescriptor(target, key) { + if (target._storeOptionsAsProperties && !(key in target)) { + target = target._optionValuesProxy; + } + return Reflect.getOwnPropertyDescriptor(target, key); + }, + ownKeys(target) { + const result = Reflect.ownKeys(target); + if (target._storeOptionsAsProperties) { + result.push(...Reflect.ownKeys(target._optionValuesProxy)); + } + return result; + }, + preventExtensions(target) { + if (target._storeOptionsAsProperties) { + Reflect.preventExtensions(target._optionValuesProxy); + } + return Reflect.preventExtensions(target); + } + }); } /** @@ -783,9 +844,6 @@ Expecting one of '${allowedValues.join("', '")}'`); */ getOptionValue(key) { - if (this._storeOptionsAsProperties) { - return this[key]; - } return this._optionValues[key]; } @@ -811,11 +869,7 @@ Expecting one of '${allowedValues.join("', '")}'`); */ setOptionValueWithSource(key, value, source) { - if (this._storeOptionsAsProperties) { - this[key] = value; - } else { - this._optionValues[key] = value; - } + this._optionValues[key] = value; this._optionValueSources[key] = source; return this; } @@ -1580,7 +1634,9 @@ Expecting one of '${allowedValues.join("', '")}'`); for (let i = 0; i < len; i++) { const key = this.options[i].attributeName(); - result[key] = key === this._versionOptionName ? this._version : this[key]; + result[key] = key === this._versionOptionName + ? this._version + : this._optionValues[key]; } return result; } From a5afe935defd7f928a236a4bf237d73d79011a50 Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Mon, 31 Jul 2023 02:29:53 +0300 Subject: [PATCH 5/5] Consistent opts() return value behavior When using options-as-properties: - Added getter and setter for value of version option - Limit setOptionValueWithSource() to registered options --- lib/command.js | 67 ++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 49 insertions(+), 18 deletions(-) diff --git a/lib/command.js b/lib/command.js index 636c06172..32978422c 100644 --- a/lib/command.js +++ b/lib/command.js @@ -589,6 +589,9 @@ Expecting one of '${allowedValues.join("', '")}'`); const oname = option.name(); const name = option.attributeName(); + // register the option + this.options.push(option); + // store default value if (option.negate) { // --no-foo is special and defaults foo to true, unless a --foo option is already defined @@ -600,9 +603,6 @@ Expecting one of '${allowedValues.join("', '")}'`); this.setOptionValueWithSource(name, option.defaultValue, 'default'); } - // register the option - this.options.push(option); - // handler for cli and env supplied values const handleOptionValue = (val, invalidValueMessage, valueSource) => { // val is null for optional option used without an optional-argument. @@ -829,10 +829,18 @@ Expecting one of '${allowedValues.join("', '")}'`); */ storeOptionsAsProperties(storeAsProperties = true) { - this._storeOptionsAsProperties = !!storeAsProperties; if (this.options.length) { throw new Error('call .storeOptionsAsProperties() before adding options'); } + if (Object.keys(this._optionValues).length) { + throw new Error('call .storeOptionsAsProperties() before setting option values'); + } + if (!this._storeOptionsAsProperties && storeAsProperties) { + this._defineVersionOptionAsProperty(); + } else if (this._storeOptionsAsProperties && !storeAsProperties) { + this._deleteVersionOptionProperty(); + } + this._storeOptionsAsProperties = !!storeAsProperties; return this; } @@ -869,6 +877,20 @@ Expecting one of '${allowedValues.join("', '")}'`); */ setOptionValueWithSource(key, value, source) { + if (this._storeOptionsAsProperties) { + if (key === this._versionOptionName) { + throw new Error(`Tried to set value of option ${key} reserved for version number. +Set version number by calling .version() instead`); + } + const optionSupported = this.options.some( + option => key === option.attributeName() + ); + if (!optionSupported) { + throw new Error(`Tried to set value of not supported option ${key}. +This is not allowed when option values are stored as instance properties. +Add support for option by calling .option() or .addOption() first`); + } + } this._optionValues[key] = value; this._optionValueSources[key] = source; return this; @@ -1627,20 +1649,6 @@ Expecting one of '${allowedValues.join("', '")}'`); * @return {Object} */ opts() { - if (this._storeOptionsAsProperties) { - // Preserve original behaviour so backwards compatible when still using properties - const result = {}; - const len = this.options.length; - - for (let i = 0; i < len; i++) { - const key = this.options[i].attributeName(); - result[key] = key === this._versionOptionName - ? this._version - : this._optionValues[key]; - } - return result; - } - return this._optionValuesProxy; } @@ -1893,7 +1901,9 @@ Expecting one of '${allowedValues.join("', '")}'`); flags = flags || '-V, --version'; description = description || 'output the version number'; const versionOption = this.createOption(flags, description); + if (this._storeOptionsAsProperties) this._deleteVersionOptionProperty(); this._versionOptionName = versionOption.attributeName(); + if (this._storeOptionsAsProperties) this._defineVersionOptionAsProperty(); this.options.push(versionOption); this.on('option:' + versionOption.name(), () => { this._outputConfiguration.writeOut(`${str}\n`); @@ -1902,6 +1912,27 @@ Expecting one of '${allowedValues.join("', '")}'`); return this; } + /** + * @api private + */ + _defineVersionOptionAsProperty() { + return Reflect.defineProperty(this._optionValues, this._versionOptionName, { + get: () => this._version, + set: (value) => { + this._version = value; + }, + configurable: true, + enumerable: true + }); + } + + /** + * @api private + */ + _deleteVersionOptionProperty() { + return Reflect.deleteProperty(this._optionValues, this._versionOptionName); + } + /** * Set the description. *