Appearance import convention

[rachelbt]

Now we are using this:

type SelectAppearance = Extract<Appearance, Appearance.Outlined | Appearance.Ghost>;
type SelectShape = Extract<Shape, Shape.Rounded | Shape.Pill>;

Suggested:

type SelectAppearance = Appearance.Outlined | Appearance.Ghost;
type SelectShape = Shape.Rounded | Shape.Pill;

@olaf-k :
because I've seen that pattern in different places: Extract<T, U> is useful if you want the intersection of T and U. if you only want some a subset of T, just take what you need.
type SelectAppearance = Extract<Appearance, Appearance.Outlined | Appearance.Ghost>;

is less readable than

type SelectAppearance = Appearance.Outlined | Appearance.Ghost;

@yinonov
because it makes the reader wonder what Extract does, find out what it does, realize it's not needed and wonder why it is used. The second is idiomatic TS and can be read with minimum TS knowledge.

original rised here: #830 (comment)

[yinonov]

1. I don't see how this declarative constructor be challenging to engineers
2. Extract makes a case for ergonomics as it wouldn't allow any other unions outside the type

[olaf-k]

It's challenging because it's unnecessary code that makes the reviewer wonder why it is used. As for the second point, yes it is useful for union types, not enums.

I didn't expect this to become a thing, so let me elaborate:

suppose you have two union types in your base code:

type US_SIZES = 'm' | 'l' | 'xl' | 'xxl';
type EU_SIZES = 'xs' | 's' | 'm' | 'l';

now, later on in your code, you could use:

type COMMON_SIZES = Extract<US_SIZES, EU_SIZES>;

which is a nice way to constrain a variable to values that are for fine for both the US and the EU.

now, you could also write something like:

type COMMON_SIZES = Extract<US_SIZES, 'm' | 'l' | 'oops' >;

which would have the same result because, as you mentioned, Extract would not allow oops since it's not in US_SIZES.
however, at this point, I would point out that TS does static typing. so values are all known at compile time and we should know that oops shouldn't be here in the first place.

finally, what we do is not intersecting two union types, but restricting an enum:

type SelectAppearance = Extract<Appearance, Appearance.Outlined | Appearance.Ghost>;

the validation done for union types is irrelevant here since members are all part of the same Appearance namespace. unknown members are just not allowed as you type:

what our code says is "give me the common values between Appearance and two of its values", which is a long a unnecessary complicated way of saying "give me two of Appearance values", which can simply be written as:

type SelectAppearance = Appearance.Outlined | Appearance.Ghost;

[yinonov]

this is an implementation detail, we SHOULD be able to trust ourselves on aligning to writing good code. it's unlikely, but a union convention, as mentioned before, will allow the following patch if author decides to deviate.

type SelectAppearance = Appearance.Outlined | Appearance.Ghost | 'AuthorCustomType';

This will not be possible with extract.

anyway this is a matter of trust in authors and has a very little affect. if we decide in favor of union, it will need to be done all at once to solidify the new convention.

@Vonage/vivid-engineering feel free to vote on your preferred convention

[olaf-k]

This will not be possible with extract.

if instead of this JS

const stuff = [ obj.prop1, obj.prop2 ];

I were to add a custom extract() function and write

const stuff = [ extract(obj, 'prop1', 'prop2') ];

you'd probably not let it fly in your code review (and TS's Extract is even worse because it's static analysis).

edit: sorry to be so persistent. I really believe removing dead code makes maintenance easier. when I wrote that Extract made the code confusing I really meant "I, a senior dev, found it confusing".