Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow readonly array as parameter of .choices() #1667

Merged
merged 5 commits into from
Jan 4, 2022
Merged

Allow readonly array as parameter of .choices() #1667

merged 5 commits into from
Jan 4, 2022

Conversation

ouuan
Copy link
Contributor

@ouuan ouuan commented Dec 30, 2021

Problem

Without readonly, readonly arrays cannot be passed as parameters.

Example: https://www.typescriptlang.org/play?#code/GYVwdgxgLglg9mABMAjACgIYCcsC5EDOUWMYA5gNoC6AlIgN4C+AUKJLAsgEyY75YBTDABMEAGwCehYqUq0GLZhARFE2LCkQBeRBQDkGPVQDcSlVDU4u23QaNqCiZWCKnW6dShqngPT9-deLC4A3yCQ4yA

Solution

Add readonly in the TypeScript typings.

ChangeLog

Allow readonly arrays as parameters of .choices().

@shadowspawn
Copy link
Collaborator

I have been doing a bit of reading and still thinking about this. Just changing the type does allow passing a ReadonlyArray as parameter, and reflects the intent that the call itself will not change the array, and lets the caller prevent casual changes to their reference to the array. However, the Commander code effectively removes the readonly in the implementation, so it is weak!

The array is stored in argChoices:

argChoices?: string[];
choices(values) {
  this.argChoices = values;
  ...
  }

The readonly could be honoured by taking a defensive copy of the parameter:

choices(values) {
  this.argChoices = Array.isArray(values) ? values.slice() : values;
  ...
  }

Reference:

Related:

  • aliases(), but in this case the implementation does take a copy

@shadowspawn
Copy link
Collaborator

I don't think readonly can be used for parameters in JSDoc?

@ouuan
Copy link
Contributor Author

ouuan commented Dec 31, 2021

this.argChoices = Array.isArray(values) ? values.slice() : values;

Could you elaborate on why Array.isArray(values) can be false?

@shadowspawn
Copy link
Collaborator

Could you elaborate on why Array.isArray(values) can be false?

Hmm, good question. Probably not needed! I was being paranoid, and had in mind perhaps passing undefined or null to remove the choices, although that is not supported either in the code or in the typings.

@shadowspawn shadowspawn changed the base branch from develop to release/9.x December 31, 2021 04:30
@shadowspawn
Copy link
Collaborator

For interest, what is your use case? (Why do you have a readonly array of choices?)

@ouuan
Copy link
Contributor Author

ouuan commented Dec 31, 2021

For interest, what is your use case? (Why do you have a readonly array of choices?)

I use a library that provides a union type for valid options. The library doesn't provide an array for the valid options, so I created a readonly array in my own code that contains these options and type-checked the array against the union type provided by the library.

lib/argument.js Outdated
@@ -97,7 +97,7 @@ class Argument {
*/

choices(values) {
this.argChoices = values;
this.argChoices = values.slice();
this.parseArg = (arg, previous) => {
if (!values.includes(arg)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The uses of values in the parse function should be replaced with this.argChoices now they may be different.

lib/option.js Outdated
@@ -117,7 +117,7 @@ class Option {
*/

choices(values) {
this.argChoices = values;
this.argChoices = values.slice();
this.parseArg = (arg, previous) => {
if (!values.includes(arg)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The uses of values in the parse function should be replaced with this.argChoices now they may be different.

@shadowspawn
Copy link
Collaborator

I offered some tests in a PR back to the branch to check the parameter is actually treated as readonly at runtime.

Copy link
Collaborator

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this will be widely needed and requires taking a defensive copy of param, but do I like making the TypeScript API more usable (readonly) and safer (defensive copy).

Thumbs up from me. 👍

Copy link
Collaborator

@abetomo abetomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@shadowspawn shadowspawn merged commit ad640de into tj:release/9.x Jan 4, 2022
@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Jan 4, 2022
@shadowspawn shadowspawn added this to the Commander v9.0.0 milestone Jan 4, 2022
@ouuan ouuan deleted the choices-allow-readonly-array branch January 4, 2022 07:06
@shadowspawn shadowspawn removed the pending release Merged into a branch for a future release, but not released yet label Jan 29, 2022
@shadowspawn
Copy link
Collaborator

Commander v9 has been released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants