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 document to match with current types #679

Merged
merged 14 commits into from
Jun 7, 2018

Conversation

Ailrun
Copy link
Member

@Ailrun Ailrun commented May 26, 2018

What: Update typescript document

Why: Typings are changed, so document is outdated.

Checklist:

  • Documentation
  • [N/A] Tests
  • [N/A] Code complete

This document depends on #678 and #680, so please do not merge this before them.

Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

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

Thanks for this!!

Could you move the create-emotion and create-emotion-styled parts to the bottom and move the parts about type checking objects into the emotion section since the vast majority of users will only use emotion and react-emotion so they shouldn't have to care about create-emotion and create-emotion-styled?

`;
```

Typescript checks css properties in object style syntax using csstype package, so following code will emit errors.
Copy link
Member

Choose a reason for hiding this comment

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

Could you remove the bit about using csstype since it's an implementation detail and people don't need to know about it.

Copy link
Member Author

Choose a reason for hiding this comment

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

But it would be helpful when users meet some type issues.... wouldn't it?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, makes sense, could you make it a link to the project as well then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh... you're right. I will add the link too.


## create-emotion-styled

Current typing for `create-emotion-styled` is only compatible with React, and will not work with Preact. For detail typing, see following `react-element` section.
Copy link
Member

@emmatown emmatown May 26, 2018

Choose a reason for hiding this comment

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

Could you change "Current typing for create-emotion-styled is only" to "The current typings for create-emotion-styled are only"?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. Thank you for your review!

@@ -2,9 +2,77 @@
title: "Typescript"
---

Emotion includes TypeScript definitions for `styled` components and has type inferences for both html elements and React components.
Emotion includes TypeScript definitions for `create-emotion`, `emotion`, `create-emotion-styled`, and `react-emotion` packages. These definition also could infer types for css property (with object syntax), and HTML/SVG tag name, and props types.
Copy link
Member

Choose a reason for hiding this comment

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

Is there a circumstance where it can't infer types for css properties?
If not, could you replace "These definition also could infer types for css property" with "These definitions also infer types for css properties"?

Also, could you replace "HTML/SVG tag name" with "HTML/SVG tag names"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Allright. I will change them.

@Ailrun Ailrun force-pushed the docs/typescript branch 2 times, most recently from ef53d27 to d02f84e Compare May 26, 2018 07:14

## preact-emotion

Types are almost same with `react-emotion` package.
Copy link
Member

Choose a reason for hiding this comment

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

If they're almost the same and not the exact same, could you explain how they're different?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. I will add following.

However, this typing use Preact component types like ComponentConstructor and FunctionalComponent instead of React component types.

Is this enough?


## create-emotion-styled

The current typings for `create-emotion-styled` are only compatible with React, and will not work with Preact. For detail typing, see `react-element` section above.
Copy link
Member

Choose a reason for hiding this comment

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

This isn't true anymore about not supporting Preact, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it still does not work with Preact, since we only have preact-emotion typing with Preact, and does not have create-emotion-styled typing with Preact.

Copy link
Member Author

@Ailrun Ailrun May 28, 2018

Choose a reason for hiding this comment

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

FYI, Preact and React both modifies JSX.Element, so we cannot import both typings in one file. That's why I did not provide typing for preact version of create-emotion-styled. (Its index typing file already import React typing)

Copy link
Member Author

Choose a reason for hiding this comment

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

The only way I can image, to deal with the problem, is, providing index typing without import any (P)React related typings and making users import them.

However, this may confuse users, because it will not work properly when users do not import (P)React related typings.

### Define a Theme

By default, the `props.theme` has `any` type annotation and works without error.\
However, you can define a theme type by creating a another `styled` instance.

_styled.tsx_
_styled.jsx_
Copy link
Member

Choose a reason for hiding this comment

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

Why the change in extension, isn't tsx generally used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, my mistake. Sorry. I will change these.

const Image = styled<ImageProps, 'div'>('div')`
background: url(${props => props.src}) center center;
const Image0 = styled('div')`
width: ${(props: ImageProps) => props.width};

Choose a reason for hiding this comment

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

Why are you using width: ${(props: ImageProps) instead of typing the component with styled<ImageProps>('div')? This is a bit confusing for me, since I see the latter as a much clearer way to type your component.

Copy link
Member Author

@Ailrun Ailrun May 29, 2018

Choose a reason for hiding this comment

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

Because in (will-be-)updated typings, there're no type parameters Props for styled.

Copy link
Member Author

@Ailrun Ailrun May 29, 2018

Choose a reason for hiding this comment

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

In TS 2.9 (which is in RC), you can use template string generic, however, before it, you should give a type to function argument and let TS infer correct type for component.

Choose a reason for hiding this comment

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

Why? Typing every prop function with ${(props: ImageProps) is more work, right? The redundant div in styled<ImageProps, 'div'> is a bit frustrating, but it will not be necessary anymore as soon as TypeScript 2.9 is released.

Copy link
Member Author

@Ailrun Ailrun May 29, 2018

Choose a reason for hiding this comment

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

In 2.9, you also don't need to type every function with new typings, since, as I said, you can use template string generic.
Before 2.9 with old typings, you should type <..., 'div'>, and sometimes even more complex things in second type parameter, and it could make really hard to type complex and large app.

The problem what you mentioned occurs only when you use more than two function arguments, and as far as I experienced, it's not that common... (Of course, this is totally my subjective opinion)

Choose a reason for hiding this comment

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

what you mentioned occurs only when you use more than two function argument, and as far as I experienced, it's not that common

In my experience I have a lot of these functions, if you are building a component library you can easily have ten of those prop functions. But if this isn't relevant anymore with TS 2.9 I digress.

Copy link
Member Author

@Ailrun Ailrun May 29, 2018

Choose a reason for hiding this comment

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

Maybe my opinion is from my object syntax based experience. Even when I made a library, I rarely used more than two function arguments.
(Just one or two functions those return proper object styles)

Copy link
Member Author

Choose a reason for hiding this comment

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

And, thank you for your feedback!

@felipeleusin
Copy link

Hi folks,

I just hit an issue. I have a button component that uses custom props but fallsback to theme props. But if I use

(props: ButtonProps) => whatever

I lose typing to Themed. I've been trying to get this to work and failing. Do you have any pointers? Should I make something Themed for this cases? Maybe it's a good thing to add in the docs

@Ailrun
Copy link
Member Author

Ailrun commented Jun 13, 2018

@felipeleusin If you use TS >= 2.9, you can use something like

styled('div')<ButtonProps>`
  ${props => something}
`;

@felipeleusin
Copy link

Oh, ok! Will try this. Thanks a lot!

@Ailrun
Copy link
Member Author

Ailrun commented Jun 13, 2018

I should add that to the document...

@felipeleusin
Copy link

I can add a PR for that, was just checking it's working first :)

@Ailrun
Copy link
Member Author

Ailrun commented Jun 13, 2018

@felipeleusin Oh, I'm not start yet. Currently I'm discussing about typing in #721.

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

4 participants