feat: add string literal union types to component attributes that use option enums

[hawkticehurst]

Pull Request

📖 Description

This PR adds inline string literal union types to all foundation components that use an enum for attribute options. This should fix an issue where downstream React-wrapped component libraries built with fast-foundation are forced to expose these enums for users to consume and use when defining a handful of attribute values.

For example, this currently does not work:

// This does NOT work (i.e. TS Error: Type '"password"' is not assignable to type 'TextFieldType | undefined'.)
// TypeScript is looking for the enum type and will not allow a literal string value
<VSCodeTextField type="password"></VSCodeTextField>

Instead, the attribute value must be defined with an enum:

import {TextFieldTypes} from "toolkit";
<VSCodeTextField type={TextFieldTypes.password}></VSCodeTextField>

This PR updates attribute type annotations so that both examples shown above are valid.

🎫 Issues

This pull request resolves #5494

👩‍💻 Reviewer Notes

I'm currently opening this as a draft PR in order to get some initial feedback before wrapping up the PR. In particular, I'd love feedback on:

• Whether these string literal types should be inlined on attribute definitions or abstracted into a type alias like discussed in feat: export component option enums as string literal union types #5494?

  The reason I opted to have these types be inline is that it will result in much better intellisense for downstream users of the components versus the abstracted type. As an example:

  With inline types I can quickly grep what the available options for a given attribute are (i.e. "above" and "below"):
  

  If the literal types are abstracted into a type alias then component users will have to either dive deeper into the type information and/or look at the component documentation to discover what's allowed.
  

  With all this said I can see there being arguments in favor of using a type alias (such as maintainability in the cases where these string literal unions are used in more than one place). So I would love feedback on what direction you would like to go as maintainers.

• Since this PR only changes type annotations, how should I go about testing this beyond making sure everything still builds correctly?

  I have my own plans to create a local build from these changes and copy-pasta the build dir into a sample React application that confirms these changes have been fixed with the toolkit React components. I'm happy to share this sample with all of you to review.

  Beyond that, however, I'm wondering if there's a preferred method of testing this that you would like to see?

• I've also had some async discussions with @chrisdholt about this PR that are worth discussing as well.

  In particular, there were thoughts about whether enums should just be deprecated at this point or not. I think we casually landed on the answer being no, but open to hearing more thoughts.

  Also if we don't deprecate enums, might there be a way to simplify the overall type annotations so that the string literal union accepts the enum as part of the union? (I'm sure I've butchered what you were thinking of for this idea so please feel free to correct me @chrisdholt).

📑 Test Plan

Reference above question about testing strategy.

✅ Checklist

General

• I have included a change request file using $ yarn change
• I have added tests for my changes.
• I have tested my changes.
• I have updated the project documentation to reflect my changes.
• I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

• I have added a new component
• I have modified an existing component
• I have updated the definition file
• I have updated the configuration file

⏭ Next Steps

Once the above questions are answered and we align on a direction for how this should be implemented/what to implement I'll finish up this work and add commits such as the change request.

[nicholasrice]

Thanks @hawkticehurst! A few thoughts the discussion points you brought up:

Inline vs Abstracted Type Unions
I think the ease of usability of the inlined types makes me prefer that approach. If folks need to reference the type, they can always use ElementCtor['propertyName'] as the type reference or the enum if we decide to keep it.

Deprecate Enums
My first instinct was to deprecate so that we don't need to keep both synchronized, but I'm curious what ya'll discussed that made you prefer keeping them.

Overall this looks great, thanks for putting it up!

[hawkticehurst]

Thanks @hawkticehurst! A few thoughts the discussion points you brought up:

Inline vs Abstracted Type Unions I think the ease of usability of the inlined types makes me prefer that approach. If folks need to reference the type, they can always use ElementCtor['propertyName'] as the type reference or the enum if we decide to keep it.

Deprecate Enums My first instinct was to deprecate so that we don't need to keep both synchronized, but I'm curious what ya'll discussed that made you prefer keeping them.

Overall this looks great, thanks for putting it up!

Thanks for the input @nicholasrice!

As I look back at my conversation with @chrisdholt it looks like the major concern was just around whether removing enums would cause a breaking change anywhere.

For example, this will definitely cause a breaking change in the toolkit, however, I'm slightly less concerned about this because we're still pre-1.0 so there's some flexibility to make this kind of change.

The other point of consideration is that these enums are used a lot within FAST Foundation as an implementation detail (on top of being used for type annotations).

// Example of the ComboboxAutocomplete enum being used as an implementation detail
private get isAutocompleteInline(): boolean {
    return (
        this.autocomplete === ComboboxAutocomplete.inline || this.isAutocompleteBoth
    );
}

private get isAutocompleteList(): boolean {
    return this.autocomplete === ComboboxAutocomplete.list || this.isAutocompleteBoth;
}

private get isAutocompleteBoth(): boolean {
    return this.autocomplete === ComboboxAutocomplete.both;
}

Obviously, these can all be changed to literal string values but depending on your opinions as maintainers there could be an argument that using literal strings is more brittle/error-prone than using enums.

Finally, I think the last consideration was just around timing. We're tentatively hoping to release v1.0 of the toolkit on April 4th so keeping this PR scoped to just adding inline literal types and not touching the enums at this time makes it a bit easier to get this work merged.

[nicholasrice]

@hawkticehurst I see, thanks for catching me up there. Removing the enums would certainly be a breaking change, but marking them deprecated could happen w/o any changes to code or exports. I'll propose moving the enum deprecation discussion to a separate issue that we can follow up on. That will unblock this PR and also give the community more opportunity to comment on the usefulness of the enums. Does that sound suitable?

[hawkticehurst]

@hawkticehurst I see, thanks for catching me up there. Removing the enums would certainly be a breaking change, but marking them deprecated could happen w/o any changes to code or exports. I'll propose moving the enum deprecation discussion to a separate issue that we can follow up on. That will unblock this PR and also give the community more opportunity to comment on the usefulness of the enums. Does that sound suitable?

Ahhh omg 🤦🏻‍♂️

I was totally conflating deprecation with removal this entire time. Yes, in that case, moving the deprecation conversation to a different PR sounds perfect. Thanks for clarifying

[hawkticehurst]

Okay, I was able to create a small demo that tests if these changes work in the Webview UI Toolkit (they do 🥳). Feel free to check it out and follow the directions in the README to validate the changes for yourself.

I also regenerated the API extractor report file, ran yarn change, and merged in the latest changes from main.

Beyond the lingering question about whether further testing strategies are preferred, I'm opening this up for a proper review!

[nicholasrice]

Looks good to me!