Skip to content

Add TypeScript types to the React integration #228

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Aug 27, 2018
Merged

Add TypeScript types to the React integration #228

merged 6 commits into from
Aug 27, 2018

Conversation

j-f1
Copy link
Contributor

@j-f1 j-f1 commented Jul 19, 2018

This adds TypeScript types to @githubprimer/octicons-react, which allows TypeScript projects to install the module and immediately start using it without installing a separate package. IMO this package should have the types integrated because they need to be generated while building the package to get the most current list of names to export.

The TypeScript types include the exact values for each icon’s size property, and calling getIconByName with a literal string representing an icon will return the correct type of that icon.

Ref desktop/desktop#5185.

@j-f1
Copy link
Contributor Author

j-f1 commented Jul 19, 2018

You must set FIGMA_TOKEN to a Figma personal access token.

Is there a way to disable that on PR builds?

@shawnbot
Copy link
Contributor

Hi @j-f1, and thanks for this! I agree that we shouldn't need FIGMA_TOKEN for CI builds when nothing has changed. Let me see if I can figure out a fix for that. In the meantime, would you mind demoing the types in a gist or something? I haven't used TypeScript before and would just like to see what they look like.

@shawnbot
Copy link
Contributor

Also, are there conventions around unit testing TypeScript types? I just want to be sure that if we do any heavy refactoring of how the icon components are generated we can ensure that we haven't broken the types. Thanks again!

@akinsho akinsho mentioned this pull request Jul 22, 2018
Merged
@liamdawson
Copy link

liamdawson commented Jul 27, 2018 •
edited
Loading

Here's a gist showing them in use. I pulled the commit in this PR in and tested it locally, and made this. (I was testing it in a Storybook).

It'd be a great help if we can get this merged.

(In the interim, I've compiled the types for 8.0.0 into a d.ts here: https://gist.github.com/liamdawson/146d8907c89e43ede2efecc108d7bb77)

@liamdawson
Copy link

Also, are there conventions around unit testing TypeScript types?

Probably define some Typescript examples with strict compiler options and check if the examples compile?

shiftkey
shiftkey reviewed Jul 27, 2018
View reviewed changes
@shiftkey
Copy link

@shawnbot

Also, are there conventions around unit testing TypeScript types?

The typical example for JS projects shipping TS declarations is essentially what moment.js has setup:

@j-f1
Copy link
Contributor Author

j-f1 commented Jul 27, 2018

I’ll try to get to this today or tomorrow.

j-f1 added 4 commits July 27, 2018 15:56
@j-f1
Move @types/react to devDependencies
6ed5831
@j-f1
Rename IconsByName to match the actual export
1307238 
Although this is a little confusing, it allows you to import `{iconsByName}` and use as both a value (`iconsByName[foo]`) and as a type (`(icon: keyof iconsByName) => boolean`).
@j-f1
Add a createIcon function to make it easier to create custom icons
5b21c0d 
Also export `OcticonProps`.
@j-f1
Add TypeScript tests
b7ce7fd
@j-f1
Copy link
Contributor Author

j-f1 commented Aug 9, 2018

This should be ready to go. Are there any other comments?

@jonrohan
Copy link
Member

@j-f1, I think it looks good, I'll let @shawnbot take care of it. He's out until next week.

@j-f1
Copy link
Contributor Author

j-f1 commented Aug 20, 2018

Friendly ping @shawnbot :)

@shawnbot
Copy link
Contributor

Hi @j-f1, this is on my list for today or tomorrow! I have every intention of getting this merged this week and deployed ASAP. Thank you for your patience, and all the hard work! ❤️

shawnbot
shawnbot approved these changes Aug 24, 2018
View reviewed changes
Copy link
Contributor

@shawnbot shawnbot left a comment

Choose a reason for hiding this comment

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

This is rad, thank you so much @j-f1!

@shawnbot shawnbot merged commit cbf33c9 into primer:master Aug 27, 2018
@shawnbot shawnbot mentioned this pull request Dec 20, 2018
Closed
@j-f1 j-f1 deleted the add-types branch March 28, 2019 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shiftkey shiftkey shiftkey left review comments

+1 more reviewer

@shawnbot shawnbot shawnbot approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@shawnbot shawnbot

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@j-f1 @shawnbot @liamdawson @shiftkey @jonrohan