remove component enums in favor of const objects #5930

chrisdholt · 2022-05-03T02:14:23Z

Pull Request

📖 Description

This PR updates existing enums for components and converts them to a const object + supported type values.

🎫 Issues

fixes #5765

👩‍💻 Reviewer Notes

📑 Test Plan

✅ 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

github-actions · 2022-05-03T02:32:55Z

Azure Static Web Apps: Your stage site is ready! Visit it here: https://purple-ocean-0b7ce3410-5930.centralus.azurestaticapps.net

github-actions · 2022-05-03T02:42:45Z

Azure Static Web Apps: Your stage site is ready! Visit it here: https://purple-ocean-0b7ce3410-5930.centralus.azurestaticapps.net

radium-v · 2022-05-03T03:00:42Z

packages/web-components/fast-foundation/src/accordion/README.md

+| `AccordionExpandMode` | Expand mode for Accordion | `{
+    /**      * Designates only a single {@link @microsoft/fast-foundation#(AccordionItem:class) } can be open a time.      */
+    single: "single",
+
+    /**      * Designates multiple {@link @microsoft/fast-foundation#(AccordionItem:class) or AccordionItems} can be open simultaneously.      */
+    multi: "multi",
+}` |


The formatting for these will need to be handled in the CEMToMarkdown script

I think it should be but definitely odd that it's not. I'll try running everything again locally - not sure why this wouldn't transform.

OH - These are the new values coming in. I'll take a look before tagging Bill :)

@williamw2 Looks like we may need some custom handling to enable proper docs for moving from enums to const objects - is that something you might be able to help with?

I'll take a look.

Per @williamw2 I think we'll need some kind of additional formatting solution here. I don't think we should block on this, but we should figure out that solution to resolve the formatting issue.

github-actions · 2022-05-03T03:04:48Z

Azure Static Web Apps: Your stage site is ready! Visit it here: https://purple-ocean-0b7ce3410-5930.centralus.azurestaticapps.net

github-actions · 2022-05-03T18:54:40Z

Azure Static Web Apps: Your stage site is ready! Visit it here: https://purple-ocean-0b7ce3410-5930.centralus.azurestaticapps.net

github-actions · 2022-05-03T18:56:34Z

Azure Static Web Apps: Your stage site is ready! Visit it here: https://purple-ocean-0b7ce3410-5930.centralus.azurestaticapps.net

KingOfTac · 2022-05-06T14:14:25Z

Just out of curiosity, I'm wondering why const objects are more preferable to enums.

chrisdholt · 2022-05-10T17:34:42Z

Just out of curiosity, I'm wondering why const objects are more preferable to enums.

There are a few reasons, but a big one is that there isn't (yet) the concept of enums in JavaScript and we've had feedback from folks who have ran into difficulty not being able to leverage certain values because of that. Additionally, enums don't really compress, so while a few may not cause much distress, creating a pattern around enums can result in "death by 1000 cuts" on the performance side of things. There is a pretty exhaustive resource here from Typescript on Enums which is worth a look; deep linking specifically to the objects vs. enums example but the whole doc is worth reading IMO: https://www.typescriptlang.org/docs/handbook/enums.html#objects-vs-enums

@latest

# Pull Request ## 🤨 Rationale We want to keep our dependencies on FAST up to date. There are a couple of issues that depend on fixes. e.g. #543, #523 ## 👩‍💻 Implementation 1. npm install all of the @microsoft/fast... packages @latest (as of late last week) 2. Fix a test failure in nimble-components drawer.spec.ts. We were firing a custom `cancel` event but [FAST started firing their own](microsoft/fast#5120), resulting in 2 events instead of the expected 1. See discussion on this PR for details. (thanks @msmithNI for investigating!) 3. Fix a type error in nimble-text-area.directive.spec.ts. This was a result of FAST's change to use const objects and type unions instead of enums microsoft/fast#5930 (it made it so the computed type of that field was the specific value of the enum rather than the type of the enum; the fix widens the type). We'll update the rest of the codebase to use this pattern and address #537 in a follow-up PR #546 since it's a pretty large change. ## 🧪 Testing Mostly relying on pipeline with some light manual testing of storybook. ## ✅ Checklist - [x] I have updated the project documentation to reflect my changes or determined no changes are needed.

# Pull Request ## 🤨 Rationale FAST decided to stop using enums and use const objects instead microsoft/fast#5930. We want to align with them so that we have consistent APIs (and for reasons outlined in the issue below). Fixes #537. ## 👩‍💻 Implementation 1. Copy the pattern that FAST used where an enum becomes a const object to store the values and a type union for the type (see `types.ts` files in the PR for examples). 3. Type union has the same name as the const object to simplify imports for clients, so many of our `FooAttribute` types are renamed to `Foo`. 4. Renum enum items to be pascalCase to align with FAST in naming too. Update callers. 5. Fixed Angular directive tests which failed because the inferred type of component harness fields was a single value (e.g. `ButtonAppearance.Outline`) instead of the type (`ButtonAppearance`) so re-assigning the field would fail. 6. Add lint rules to enforce no enums and to allow the types to be PascalCase and values to be camelCase. Applied the naming rules only to `types.ts` to prevent them from affecting other objects. 7. Moved some existing enums into `types.ts` files rather than next to their usages so that the lint rules apply 8. Updated some matrix tests to re-capitalize enum names when they're displayed on component labels; this avoids a visual diff and generally looks better. 9. Added CONTRIBUTING docs explaining enum rules ## 🧪 Testing Relying on pipeline. ## ✅ Checklist  - [x] I have updated the project documentation to reflect my changes or determined no changes are needed.

chrisdholt requested review from EisenbergEffect, janechu, nicholasrice, awentzel and Falkicon as code owners May 3, 2022 02:14

chrisdholt mentioned this pull request May 3, 2022

fix select dropdown positioning #5811

Merged

6 tasks

chrisdholt force-pushed the users/chhol/remove-enums-in-favor-of-const-objects branch from 7936de5 to c64e73a Compare May 3, 2022 02:25

chrisdholt force-pushed the users/chhol/remove-enums-in-favor-of-const-objects branch from c64e73a to 79f233c Compare May 3, 2022 02:46

radium-v reviewed May 3, 2022

View reviewed changes

nicholasrice approved these changes May 3, 2022

View reviewed changes

chrisdholt and others added 6 commits May 3, 2022 11:37

convert enums to const objects

95310b1

convert component class enums to const objects and add supported types

60f9ba8

Change files

0e5ab4b

convert orientation enum to const object with corresponding type

e4e9332

Change files

b4840d3

Fixed variable type formatting (#5931)

2cae9b9

chrisdholt force-pushed the users/chhol/remove-enums-in-favor-of-const-objects branch from a4ba787 to 2cae9b9 Compare May 3, 2022 18:38

EisenbergEffect approved these changes May 3, 2022

View reviewed changes

chrisdholt merged commit d392841 into master May 3, 2022

chrisdholt deleted the users/chhol/remove-enums-in-favor-of-const-objects branch May 3, 2022 19:48

rajsite mentioned this pull request May 3, 2022

Align enum format with FAST ni/nimble#537

Closed

jattasNI mentioned this pull request May 10, 2022

Update FAST dependencies ni/nimble#542

Merged

1 task

jattasNI mentioned this pull request May 10, 2022

Use const objects and type unions instead of enums ni/nimble#546

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove component enums in favor of const objects #5930

remove component enums in favor of const objects #5930

chrisdholt commented May 3, 2022

github-actions bot commented May 3, 2022

github-actions bot commented May 3, 2022

radium-v May 3, 2022

chrisdholt May 3, 2022

chrisdholt May 3, 2022

chrisdholt May 3, 2022

williamw2 May 3, 2022

chrisdholt May 3, 2022

github-actions bot commented May 3, 2022

github-actions bot commented May 3, 2022

github-actions bot commented May 3, 2022

KingOfTac commented May 6, 2022

chrisdholt commented May 10, 2022

remove component enums in favor of const objects #5930

remove component enums in favor of const objects #5930

Conversation

chrisdholt commented May 3, 2022

Pull Request

📖 Description

🎫 Issues

👩‍💻 Reviewer Notes

📑 Test Plan

✅ Checklist

General

Component-specific

⏭ Next Steps

github-actions bot commented May 3, 2022

github-actions bot commented May 3, 2022

radium-v May 3, 2022

Choose a reason for hiding this comment

chrisdholt May 3, 2022

Choose a reason for hiding this comment

chrisdholt May 3, 2022

Choose a reason for hiding this comment

chrisdholt May 3, 2022

Choose a reason for hiding this comment

williamw2 May 3, 2022

Choose a reason for hiding this comment

chrisdholt May 3, 2022

Choose a reason for hiding this comment

github-actions bot commented May 3, 2022

github-actions bot commented May 3, 2022

github-actions bot commented May 3, 2022

KingOfTac commented May 6, 2022

chrisdholt commented May 10, 2022