Skip to content

Commit

Permalink
Revert "Return proxy redirecting keys for options-as-properties from …
Browse files Browse the repository at this point in the history 
…Command constructor"

This reverts commit 1b94efb.
  • Loading branch information
@aweebit
aweebit committed Aug 3, 2023
1 parent fc927c8 commit ef54142
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 147 deletions.
172 changes: 26 additions & 146 deletions lib/command.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,71 +12,7 @@ const { suggestSimilar } = require('./suggestSimilar');

// @ts-check

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._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).filter(
key => !(result.includes(key)) // remove duplicates
));
}
return result;
},
preventExtensions(target) {
if (target._storeOptionsAsProperties) {
Reflect.preventExtensions(target._optionValuesProxy);
}
return Reflect.preventExtensions(target);
}
});
}
}

class Command extends CommandBase {
class Command extends EventEmitter {
/**
* Initialize a new `Command`.
*
Expand Down Expand Up @@ -142,18 +78,7 @@ class Command extends CommandBase {
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;

// Double proxy to show the version option property value instead of [Getter/Setter] when printing the return value of opts() to a console.
// Required because Node internally unwraps one proxy and therefore would not use the getOwnPropertyDescriptor() trap otherwise.
this._optionValuesProxy = new Proxy(new Proxy(this._optionValues, {
get: (_, key) => {
return this.getOptionValue(key);
},
this._optionValuesProxy = new Proxy(this._optionValues, {
set: (_, key, value) => {
this.setOptionValue(key, value);
return true;
Expand All @@ -168,20 +93,8 @@ Option value deletion is not supported`);
- or Object.defineProperties(),
- or Reflect.defineProperty().
Options value configuration is not supported`);
},
getOwnPropertyDescriptor: (target, key) => {
if (this._storeOptionsAsProperties && key === this._versionOptionName) {
// Mask the accessor property defined by ._defineVersionOptionProperty() as a data property.
return {
value: target[key],
writable: true,
configurable: true,
enumerable: true
};
}
return Reflect.getOwnPropertyDescriptor(target, key);
}
}), {});
});
}

/**
Expand Down Expand Up @@ -615,9 +528,6 @@ 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
Expand All @@ -629,6 +539,9 @@ 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.
Expand Down Expand Up @@ -855,20 +768,10 @@ 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._versionOptionName !== undefined) {
if (!this._storeOptionsAsProperties && storeAsProperties) {
this._defineVersionOptionProperty();
} else if (this._storeOptionsAsProperties && !storeAsProperties) {
this._deleteVersionOptionProperty();
}
}
this._storeOptionsAsProperties = !!storeAsProperties;
return this;
}

Expand All @@ -880,6 +783,9 @@ Expecting one of '${allowedValues.join("', '")}'`);
*/

getOptionValue(key) {
if (this._storeOptionsAsProperties) {
return this[key];
}
return this._optionValues[key];
}

Expand All @@ -906,20 +812,10 @@ 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[key] = value;
} else {
this._optionValues[key] = value;
}
this._optionValues[key] = value;
this._optionValueSources[key] = source;
return this;
}
Expand Down Expand Up @@ -1677,6 +1573,18 @@ Add support for option by calling .option() or .addOption() first`);
* @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[key];
}
return result;
}

return this._optionValuesProxy;
}

Expand Down Expand Up @@ -1917,7 +1825,7 @@ Add support for option by calling .option() or .addOption() first`);
*
* You can optionally supply the flags and description to override the defaults.
*
* @param {string} [str]
* @param {string} str
* @param {string} [flags]
* @param {string} [description]
* @return {this | string} `this` command for chaining, or version string if no arguments
Expand All @@ -1930,7 +1838,6 @@ Add support for option by calling .option() or .addOption() first`);
description = description || 'output the version number';
const versionOption = this.createOption(flags, description);
this._versionOptionName = versionOption.attributeName();
if (this._storeOptionsAsProperties) this._defineVersionOptionProperty();
this.options.push(versionOption);
this.on('option:' + versionOption.name(), () => {
this._outputConfiguration.writeOut(`${str}\n`);
Expand All @@ -1939,33 +1846,6 @@ Add support for option by calling .option() or .addOption() first`);
return this;
}

/**
* Expose the internal version variable via an accessor property in the option value storage.
*
* The version variable is not expected to be exposed when the modern approach to option value storage is used.
* It is only exposed for backwards compatibility when options-as-properties are enabled.
* Therefore, the method is only to be called when they are enabled.
*
* @api private
*/
_defineVersionOptionProperty() {
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.
*
Expand Down
2 changes: 1 addition & 1 deletion typings/index.d.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ export class Command {
*
* You can optionally supply the flags and description to override the defaults.
*/
version(str?: string, flags?: string, description?: string): this;
version(str: string, flags?: string, description?: string): this;

/**
* Define a command, implemented using an action handler.
Expand Down

0 comments on commit ef54142

Please sign in to comment.