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

feat(lib): allow strong typing for icons #15

Merged
merged 3 commits into from
Jan 22, 2024

Conversation

minenwerfer
Copy link
Contributor

This PR adds a const constraint to the icons array exported by src/icons.ts, so the Typescript compiler will emit a much more useful declaration file which can later be used to strictly type valid icon names. It also exports a new PhosphorIcon type.

Instead of emitting this in `dist/icons.d.ts:

export declare const icons: ReadonlyArray<IconEntry>;

With this PR the following will be emitted:

export declare const icons: readonly [{
    readonly name: "address-book";
    readonly pascal_name: "AddressBook";
    readonly categories: readonly [IconCategory.COMMUNICATION];
    readonly figma_category: FigmaCategory.COMMUNICATION;
    readonly tags: readonly ["contacts", "roledex"];
    readonly codepoint: 59128;
    readonly published_in: 1.3;
    readonly updated_in: 1.3;
}, // ...
]

Now the following becomes possible:

import { PhosphorIcon } from '@phosphor-icons/core'
declare const myComponent: (props: { icon: PhosphorIcon['name'] }) => any

myComponent({
  // this prop is strongly typed
  icon: 'address-book'
})

I believe this will hardly break anything. If it does, a separated export can be added.

@rektdeckard
Copy link
Member

rektdeckard commented Jan 21, 2024

This seems fine, and is a nice improvement. I'm curious about devtime performance — does LSP still feel snappy with a hefty mapped type like PhosphorIcon? I have heard that big unions really thrash typechecking. Will give it a try later.

@minenwerfer
Copy link
Contributor Author

This seems fine, and is a nice improvement. I'm curious about devtime performance — does LSP still feel snappy with a hefty mapped type like PhosphorIcon? I have heard that big unions really thrash typechecking. Will give it a try later.

It feels normal in Neovim + LSP. Not super fast, not super slow either.

@rektdeckard
Copy link
Member

Feels good on my end. Please also update the codegen script that creates the icons.ts file, then run the catalog script to emit the file rather than changing it manually:

https://github.com/phosphor-icons/core/blob/main/scripts/catalog.ts#L41-L107

@minenwerfer
Copy link
Contributor Author

Feels good on my end. Please also update the codegen script that creates the icons.ts file, then run the catalog script to emit the file rather than changing it manually:

https://github.com/phosphor-icons/core/blob/main/scripts/catalog.ts#L41-L107

Sure, I think I missed that. Will commit later.

@minenwerfer
Copy link
Contributor Author

@rektdeckard I just made a new commit, please review it.

@rektdeckard rektdeckard merged commit 8e886e9 into phosphor-icons:main Jan 22, 2024
@rektdeckard
Copy link
Member

Thanks for the contrib!

@minenwerfer
Copy link
Contributor Author

Thanks for merging my PR!
Could you please release this version on npm?

@rektdeckard
Copy link
Member

I'm writing some additional docs, will publish this evening

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.

None yet

2 participants