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

Update Typescript #331

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Update Typescript #331

wants to merge 22 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Feb 14, 2020

https://trello.com/c/FkPWevtY/79-roe-typescript-upgrade

  • Upgrade to TS 3.3
  • Upgrade react-docgen-typescript
  • Fix type component

Note: Not related to TS upgrade, but also added whitespace to group imports

@nvlaarhoven nvlaarhoven force-pushed the upgrade-typescript branch 2 times, most recently from eec55f2 to a03332d Compare April 16, 2020 13:01
@nvlaarhoven nvlaarhoven changed the title Update packages Update Typescript Apr 16, 2020
src/ts/types.ts Outdated
export interface ComponentProps {
/**
* Set the component to render a different element type.
*/
component?: string;
component?: ReactType;
Copy link
Contributor

Choose a reason for hiding this comment

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

This type is deprecated (and I don't think correct). You want an explicit list of available elements really... I can't remember where I've got this from before...

You may be able to use keyof IntrinsicElements or Pick<keyof IntrinsicElements , 'div' | 'span' | 'etc etc'>.

Or just manually define them: div | span | etc etc

tests/index.tsx Outdated
@@ -49,7 +49,7 @@ describe('index file', () => {

for (const key in index) {
if (index.hasOwnProperty(key)) {
const Component = index[key as Keys];
const Component = index[key as Keys] as ReactType;
Copy link
Contributor

Choose a reason for hiding this comment

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

How come this is necessary?

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

Successfully merging this pull request may close these issues.

2 participants