Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace enums in props with union types #48

Open
mturley opened this issue Jan 19, 2021 · 4 comments
Open

Replace enums in props with union types #48

mturley opened this issue Jan 19, 2021 · 4 comments

Comments

@mturley
Copy link
Collaborator

mturley commented Jan 19, 2021

This will be a breaking change, and it's just a code cleanliness thing, but I think it'd be better to get it in sooner rather than later. Based on a discussion with @gildub a while back where he shared this article with me: https://blog.logrocket.com/put-the-typescript-enums-and-booleans-away/

It is a pain to have to import the enums used in props for certain components rather than just be able to let IntelliSense tell yo which strings are valid. TS can prevent invalid values of string types if the valid values are provided in a union type.

We should also consider watching out for the advice against booleans for state, but I'm not sure that's as important.

@gildub gildub self-assigned this Jan 20, 2021
@gildub
Copy link
Collaborator

gildub commented Jan 20, 2021

Ideally combining enums and union types get the best of both worlds.

See example https://blog.logrocket.com/put-the-typescript-enums-and-booleans-away/#comment-4566

@mturley
Copy link
Collaborator Author

mturley commented Jan 20, 2021

Hmm, I think that helps with cases where we have more than one prop and only certain combinations of those props are valid, but for this issue I was just thinking of the basic case where we have e.g. statusType: StatusType and we could instead have statusType: 'error' | 'warning' | 'info' | 'loading' so when we consume the component we don't need to import the enum and we can just pass a string, so intellisense works right away instead of waiting for you to add the import. Like the AuthenticationStates example in the article.

I'm still wrapping my head around the best-of-both-worlds approach, is the benefit of keeping enums in there just so it's more searchable?

@mturley
Copy link
Collaborator Author

mturley commented Jan 20, 2021

Also I think this is super low priority, I was just opening the issue so I don't forget about it

@gildub
Copy link
Collaborator

gildub commented Jan 20, 2021

@mturley, I was keen to work on it because it seems to be a good practice to make code more robust by enforcing types. And also because it allows me to get stronger with types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants