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

Properly infer icons type #484

Open
wants to merge 1 commit into
base: 3.x
Choose a base branch
from
Open

Properly infer icons type #484

wants to merge 1 commit into from

Conversation

Mensix
Copy link

@Mensix Mensix commented Oct 29, 2023

The library exposes IconPrefix, IconName and IconDefinition types, which can be used to improve developer experience by marking props definition with those. As the change is strictly related to TypeScript, I am not sure how to write tests for it. I hope somebody will take a look on it :)

icon: object | Array<string> | string | IconDefinition
mask?: object | Array<string> | string
icon: [IconPrefix, IconName] | IconName | IconDefinition
mask?: [IconPrefix, IconName] | IconName
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mensix --- Would/could this break backwards compatibility ?

Copy link
Author

@Mensix Mensix Nov 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good question! I tested the change with the Nuxt framework - obviously VS Code gave autocompletion properly. Once given wrong icon name, an TS related error appears, however... it means nothing at all, as typing errors are ignored both in the dev and build mode. Seems like the case also for Vite and vue-cli according to their docs, unless using vue-tsc utility.

So 🤔, I will have a further look on it tomorrow.

Copy link
Author

@Mensix Mensix Nov 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed that there is a breaking change -- string syntax wouldn't be further supported. If we try to extend types by it:

icon: [IconPrefix, IconName] | IconName | IconDefinition | [CssStyleClass, `fa-${IconName}`]
mask?: [IconPrefix, IconName] | IconName | [CssStyleClass, `fa-${IconName}`]

then (at least VSC) gives lots of pointless options, like fa-duotone fa-facebook. But!

We could also totally make the string option not supported by autocompletion, yet the rest would still work, by using, well, weird TypeScript hacks: microsoft/TypeScript#29729. But there won't be any breaking changes then ;)

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

Successfully merging this pull request may close these issues.

2 participants