feat(StatusIcon): Declarative approach using union types

[gildub]

BREAKING CHANGE: statusType prop of the StatusIcon component is now a string, and the StatusType enum is no longer exported.

Replaces string enums with unions types.
Use lookup table for icons.

#48

[konveyor-preview-bot]

🚀 Deployed Storybook Preview: http://konveyor-lib-ui-pr-49-preview.surge.sh ✨

[mturley]

I like the use of this iconList as a lookup table, but I'm not sure if it's worth the duplication of this icon-and-color-mapping information in the IconType. We have to sort of repeat the information twice here, and the type is only checking that we repeated it correctly. I think iconListType could just be

type iconListType = { [key in StatusType]: React.Component<SVGIconProps> }

which is what PF uses internally for the icon components (https://github.com/patternfly/patternfly-react/blob/master/packages/react-icons/src/createIcon.tsx#L54).

What I was referring to in the issue was perhaps replacing the StatusType enum itself with:

export type StatusType = 'Ok' | 'Warning' | 'Error' | 'Info' | 'Loading' | 'Unknown';

And then this iconList would just use regular object keys (strings) and TS would enforce that each one is a valid StatusType even without the enum, and that way when using the component you can just pass statusType="Ok" instead of having another import.

[mturley]

At least that's one of the takeaways I got from the article. Maybe I'm missing the point 😄

[gildub]

Thanks for the feedback.

I agree with the redundant icon-color mapping information.
One advantage of the IconType type is to enforce the defined icon and color mappings but maybe that's not that important here.

As it was discussed in the article's comments, we can also get best of both worlds. So by using an enum for the string values makes it easier to maintain those and exploit them with a union. Meanwhile that's might not be that relevant here.

[mturley]

Yeah, maybe I'm not fully wrapping my head around why enums are easier to maintain. You're probably right in some contexts but in this specific case I think a simple union of strings provides the same benefits.

As far as enforcing the icon/color mappings, those mappings are built into the component itself so we're not enforcing anything the consumer passes since they don't specify the color. The iconList mapping itself provides that info (the type just enforces that the object matches the type, within the same file.. it seems like it would be just as easy to make a mistake in the type than in the object itself.

I think a useful thing to enforce would be just that the iconList fully covers all the available status types, and all we need for that is:

type iconListType = {
  [key in StatusType]: {
    icon: React.Component<SVGIconProps>;
    color: { name: string; value: string; var: string };
  };
};

maybe I'm trying to over-simplify.

[gildub]

By using enums we can also use them instead of manipulating hard coded strings which was one of the initial incentive to use them:
status === StatusType.Loading vs status === "Loading". We loose that by doing type StatusIconType = "Ok" | "Warning" | "Error" | "Info" | "Loading" | "Unknown";`

Regarding the IconList, you're right the type doesn't enforce anything because the code controls it. So I might have overcomplicated it. Well it's a trial and error approach to build up with the enum/union pattern in this case.
The main issue left is to import for SVGIconProps and SpinnerProps.

[mturley]

By using enums we can also use them instead of manipulating hard coded strings

yeah, that was the original reason I liked them, but I realized after trying string unions that using hard-coded strings is just fine if there are only certain allowed ones, IntelliSense and TSC will still autocomplete/check them for you and you don't have to import the enum.

[codecov]

Codecov Report

Merging #49 (89d7d3c) into main (8dfb8b9) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #49   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           39        19   -20     
  Branches        14         5    -9     
=========================================
- Hits            39        19   -20     

Impacted Files	Coverage Δ
src/components/StatusIcon/StatusIcon.tsx	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8dfb8b9...89d7d3c. Read the comment docs.

[mturley]

I added the BREAKING CHANGE: text to the body so it will tell semantic-release to make a major version bump

[konveyor-release-bot]

🎉 This PR is included in version 1.13.0 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

[mturley]

oh weird... it didn't trigger a major bump..

[mturley]

Forced another release with a3cae11.

@gildub apparently github's squash-and-merge just combines all the commit messages and doesn't include the PR body as a commit message body :( so if we have breaking changes we need to add BREAKING CHANGES: (description) at the top of the commit message it offers us when we click merge

[mturley]

https://github.com/konveyor/lib-ui/releases/tag/v2.0.0