fix: add options field to Command type declaration

[cyyynthia]

Pull Request

Problem

The Command type for TypeScript doesn't expose the options field.

Solution

Added the options field to the Command TypeScript type declaration, with the same type as specified in the JSDoc (Option[])

ChangeLog

fix: add options field to Command type declaration

[shadowspawn]

What do you want access to the options for? (What is the use case, or the problem this solves?)

---

We could add the options to the TypeScript, and they are obviously in the JavaScript. But it won't work adding new options directly to the array, and there are some operations that won't work properly after the option has been added to the command.

The last time it got proposed, we decided not to add them at the time. See #1784

[cyyynthia]

I used this in a project to add a custom external config loader routine as part of a prehandler logic:

If a certain options were options the command takes, it would proceed to load a fallback value using custom logic. Said logic was quite expensive and depends on other options, so I ran it lazily when required. Because it was used in a few places, it made sense to me when I wrote it to load it during preHandler once, rather than each command handler dealing with post-processing the options.

Note: I couldn't have a "magic value" as default that could be used to only check the cmd.optsWithGlobals() without some quite annoying typing issues, or risking to cast/assume things I shouldn't

I can add a readonly modifier to not allow modification of the array (TypeScript will raise an error if .push or any modification is attempted on a ReadonlyArray)

[cyyynthia]

The workaround proposed in #1784 isn't the prettiest but could theoretically suffice I guess. If adding options is causing typing concerns, maybe adding a direct method on the Command object would prevent breaking changes, and let people access the options without the not-so-intuitive step of using Help.

[shadowspawn]

(Don't worry about the "extra-typings" showing up in the changes, my fault, I'll fix that.)

[shadowspawn]

1. We have had two PR for options in a few months, so I think worth adding this time.

Note to self: add @mshima as a co-author when merging this PR. (Author of #1784)

I can add a readonly modifier to not allow modification of the array (TypeScript will raise an error if .push or any modification is attempted on a ReadonlyArray)

Yes, I think adding readonly is appropriate and useful. Options should be added to the array through Command routines and not directly.

3. I would like commands and options the same, so add a readonly to that as well.

[shadowspawn]

I was thinking of options: readonly Option[] rather than readonly options: Option[], in particular to make options.push(x) an error. The test change you made suggests that was what you had in mind too. I had to go try some code to confirm I understood what each means! Or as suggested in #1184 could have both readonly options: readonly Options[].

However, looking at all the variations and what they mean semantically makes me question again whether readonly is worthwhile here!

We have not had issues raised by people breaking commands (or options) in ways that simple use of readonly would prevent. The property is there in the JavaScript. I am leaning back towards just exposing it as you initially suggested with options: Option[].

Comments welcome!

[cyyynthia]

The lack of readonly on the array type is an oopsie of mine, I should've double checked 😅 I did the edit on the go via GitHub 's online editor, and didn't check back test results 😔

I have mixed feelings about the readonly marker now that I think about it. On one hand, for user-facing types the intent is that these fields should not be modified which calls for marking it all readonly; on the other, if the library was written in TypeScript directly, because the code does mutate the options, the emitted .d.ts file would not have any readonly modifier.

For the latter point, my take would be that it's actually the advantage of hand-written declaration files: we have the possibility to reflect what should the user have access to and how, while allowing the library code itself to break these constrains that only apply to users.

I don't think there's harm in adding readonly markers where appropriate for TypeScript users, even if it's a small thing that probably won't change much.

[shadowspawn]

Yes, that matches what I am wondering too. Should the types be what we think suits consumer, or more in line with types which would be used by maintainers if library written in TypeScript.

For the latter point, my take would be that it's actually the advantage of hand-written declaration files: we have the possibility to reflect what should the user have access to and how, while allowing the library code itself to break these constrains that only apply to users.

I quite like that story. Effectively the internal implementation and external API use different accessors. (They just happen to be the same property because independent JavaScript and TypeScript.)

I don't think there's harm in adding readonly markers where appropriate for TypeScript users, even if it's a small thing that probably won't change much.

Ok, persuaded me back to continuing the readonly approach!

[shadowspawn]

Closed in favour of #1827 to fix the lint issues and acknowledge past contributors.