Resolve type incompatibilities with legacy type #722

Ailrun · 2018-06-13T14:59:28Z

What: Enhance type inference for following case.

interface Props {
  w: number;
}
declare const Comp: SFC<Props>;
const X = styled(Comp)`
  width: ${props => props.w};
`

and add type parameter for compatibilities with old type

interface Props {
  w: number;
}
const X = styled<Props, 'div'>('div')`
  width: ${props => props.w};
`

Why: #721.

How: By adding default type parameter

Checklist:

Documentation
Tests
Code complete

Resolves #721.

tkh44 · 2018-06-13T18:15:23Z

packages/create-emotion-styled/types/test.tsx

@@ -131,9 +131,9 @@ const StyledTag1 = createStyled('label')`
 const StyledCompWithButton = StyledClassComp0.withComponent('button');
 const StyledCompWithFunComp2 = StyledFunComp0.withComponent(TestFunComp1);

-<StyledCompWithButton />;


Should we keep this case?

@tkh44 This is the problem what I mentioned in #721.
We cannot distinguish props that is used to make style or props that is passed to internal component without type parameter, so there are only two ways, considering them as props for style, or considering them as a internal-only props.

#721, @mvestergaard said complaint about second choice (my original choice), so I changed this into first one.

@tkh44 Could you discuss this with @mvestergaard ?

This might be naive, but wouldn't this make that use case work?

withComponent<T extends keyof JSX.IntrinsicElements>( tag: T, options?: StyledOptions, ): StyledOtherComponent<JSX.IntrinsicElements[T], JSX.IntrinsicElements[T], Theme>;

Basically force Props to be the same as InnerProps.

@mvestergaard The problem occurs in original styles (styles those came from StyledComponent before withComponent is applied). Original styles already depend on old Props, so we cannot change them freely. If we do so, it will break the type.

Anyway, I'm not sure what i can discuss with @tkh44
I've spoken my case on #721 already. The gist is basically, that a breaking change was made. Something that worked just fine before, now requires explicit typing.
In my head this is wrong, but I can't force you guys to change it based on that. So you need to decide internally how you intend the library to be used.
The most common use case should be prioritized.

My personal opinion is that restyling components:

const Restyled = styled(OtherComponent)``

is a more common use case than switching the internal component

const SomeStyledThing = styled.div``; const SomeOtherStyledThing = SomeStyledThing.withComponent('span');

But @Ailrun believes it's the other way around

@mvestergaard Thank you for your summary!
What I mean with 'discussion' is about 'Which one is more common and preferable use case'.
How about your opinion, @tkh44?

Ailrun · 2018-06-16T23:31:03Z

@tkh44 Why did codecov coverage decrease? Does codecov coverage include .ts files?

emmatown · 2018-06-16T23:43:01Z

@Ailrun It said that because this branch was out of date with master

emmatown · 2018-06-18T09:14:23Z

docs/typescript.md

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


Should props: ImageProps be changed to props here?

@mitchellhamilton Oh, thanks! I will change it.

emmatown · 2018-06-18T09:15:07Z

docs/typescript.md

  background: url(${(props: ImageProps) => props.src}) center center;
  background-size: contain;
 `

 // Or with object styles

-const Image1 = styled('div')({
+const Image1 = styled<ImageProps, 'div'>('div')({


Could you explain why people would want to do this over the generic way below?

@mitchellhamilton Actually, I don't think this is the preferable way.... I add this because of compatibilities. If you think 'Document only for compatibility reason is not needed', I will change this.

However, in case of string style, as you already know, situation is little different, since we cannot use generic template string with TS < 2.9, and even with TS >= 2.9, vscode-styled-components does not support syntax highlight.

Could you move it below the one that people should use and add a comment about it only being there for compatibility and that people shouldn’t write new code with it?

OK, I will move this and add a comment.

emmatown · 2018-06-18T09:16:58Z

docs/typescript.md

@@ -119,7 +133,8 @@ const Image1 = styled('div')<ImageProps>({
 }));
 ```

-* The generic type version only works with object styles due to https://github.com/Microsoft/TypeScript/issues/11947.
+* The generic function version only works with object styles in TS <= 2.8 due to https://github.com/Microsoft/TypeScript/issues/11947.
+* If you use TS > 2.9, generic function will works with string styles too, but it will break VSCode syntax highlighting. See https://github.com/emotion-js/emotion/issues/721#issuecomment-396954993


Could you replace generic function will works with string styles too with generic functions will work with string styles too?

Oh, my mistake! I will fix it

emmatown · 2018-06-18T09:19:16Z

docs/typescript.md

  <div className={className}>{label}</div>
 )

 type StyledComponentProps = {
  bgColor: string
 };

-const StyledComponent0 = styled(Component)`
+const StyledComponent00 = styled<StyledComponentProps, ComponentProps0>(Component)`


Why is a prop type passed here but in other places div is passed (shouldn't this be a component type)?

@mitchellhamilton This is for compatibilities, and, sadly, old typings did so...

emotion/packages/react-emotion/types/index.d.ts

Lines 101 to 108 in 24fa6ed

<Props, Tag extends keyof JSX.IntrinsicElements>(

tag: Tag,

options?: Options,

// tslint:disable-next-line:no-unnecessary-generics

): CreateStyled<Props, Theme, ElementProps<Tag>>;

// overload for component

<Props, CustomProps>(

Is this the preferred way to do it now?
If not, could you add a note saying that it's only for compatibility and tell people what they should use now.

@mitchellhamilton
I notice that this is the preferred way.... There is no walkaround for this case, to be compatible with old one.
I will try better typing at upcoming emotion@10

emmatown · 2018-06-18T09:21:50Z

docs/typescript.md

 const App = () => <Link href="#">Click me</Link>

 // No errors!
 ```

+When you use `withComponent`, you should explicitly provide the type of props used to make style. If you don't, TS will think all props for `Text` are used to make style, and request user to give all those props when using the result component of `withComponent`.


Could you elaborate on TS will think all props for Text are used to make style, I don't really understand what it means.

@mitchellhamilton I mean TS will consider all props for Text as props to make style. Is this sentence more sensible?

No, I still don’t understand it. I don’t understand what you mean by as props to make style. how do the props make the style?

@mitchellhamilton I mean something like following

(props) => ({ backgroundColor: props.bgColor })

(bgColor props is used to make style)

For example,

When you have Text component that requires bgColor, content, you can make following styled component.

const StyledText = styled(Text)({ fontSize: '50px', }, props => ({ backgroundColor: props.bgColor, }));

In this StyledText, bgColor prop is used to make style, but content prop isn't.

Now, suppose that you want to make a new styled component with different inner (base?) component Link, by using withComponent. Suppose that Link requires only target as a prop. You probably do something like following.

const StyledLink = StyledText.withComponent(Link);

However, the problem occurs in TypeScript. TypeScript cannot determine which props are used to make style, i.e., it cannot find the fact that bgColor is the only prop used in style of StyledText. Therefore it should do one of following things.

Suppose that all props are used in style

Suppose that no props are used in style

However, if you choose first one, TypeScript request to use bgColor, content, target to StyledLink, even if Link does not require content. This is why I choose second one, but legacy typing choose first one and after I changed this, some users complain about 'Why are there no props type inference?'.

What do styled-components and glamorous do for these cases?

@mitchellhamilton Glamorous does something like second choice (not exactly same, but similar), and styled-components has something similar with first one, but with wrong typed withComponent.

props used to make style

Maybe try:

The prop types used to construct the resulting style definition of withComponent should be explicitly provided to avoid TypeScript compiler errors.

@tkh44 I just changed the docs. Could you check this again?

emmatown · 2018-06-18T09:24:08Z

packages/create-emotion-styled/types/test.tsx

@@ -50,15 +50,15 @@ const TestFunComp1 = (props: TestFunProps1) => (
  </div>
 );

-const StyledClassComp0 = createStyled(TestClassComp)({
+const StyledClassComp0 = createStyled(TestClassComp)<{}>({


Why was <{}> added?

@mitchellhamilton Now, all props of component that passed to createStyled (or styled) considered as props for styling, so if you want to use withComponent and don't use any props of passed component (or use only some of them), you should restrict props type by providing type parameter.

Ref: #722 (comment)

Ailrun · 2018-06-25T18:53:06Z

@mitchellhamilton Could you check this again?

tkh44 · 2018-06-25T19:27:38Z

I think the main hold up is around the documentation. Once we get this worked out I feel like it should be good to go.

tkh44 · 2018-06-25T23:37:17Z

docs/typescript.md

+`;
+
+const OtherLink = StyledText.withComponent('a');
+
 const App = () => <Link href="#">Click me</Link>


Should Link be OtherLink in this example?

We also have Link... Do I need to use all these components (Link, OtherLink, ...)?

emmatown · 2018-07-21T05:26:41Z

@Ailrun could you fix the conflicts?

The compatibility is about old type & styled-component

Ailrun · 2018-07-27T17:40:26Z

@mitchellhamilton Sorry for delay. I just came back from my vacation.

stale · 2018-09-25T18:21:47Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

Ailrun added Tag: Enhancement TypeScript labels Jun 13, 2018

Ailrun self-assigned this Jun 13, 2018

Ailrun requested review from mgroenhoff and emmatown June 13, 2018 14:59

Ailrun mentioned this pull request Jun 13, 2018

Typing of props in restyled components is broken in latest build #721

Closed

Ailrun force-pushed the typings/type-inference branch from bc6a2a5 to 91e43e8 Compare June 13, 2018 16:11

tkh44 reviewed Jun 13, 2018

View reviewed changes

Ailrun added the discussion label Jun 13, 2018

Ailrun changed the title ~~Enhance type inference for initially styled components~~ Resolve type incompatibilities with legacy type Jun 17, 2018

Ailrun removed the discussion label Jun 18, 2018

emmatown reviewed Jun 18, 2018

View reviewed changes

tkh44 reviewed Jun 25, 2018

View reviewed changes

Ailrun and others added 8 commits July 27, 2018 13:39

Enhance type inference for initially styled components

b01ec2f

Fix tests

1710660

Add type parameters for compatibility

9f44ba7

The compatibility is about old type & styled-component

Fix the document about typescript

c95260f

Fix typescript doc

712a828

Change typescript example order

3983eea

Update typescript document

3aba913

Update typescript.md

8f36c4f

Ailrun force-pushed the typings/type-inference branch from fd2af86 to 8f36c4f Compare July 27, 2018 17:39

stale bot added the stale label Sep 25, 2018

stale bot closed this Oct 2, 2018

	<Props, Tag extends keyof JSX.IntrinsicElements>(
	tag: Tag,
	options?: Options,
	// tslint:disable-next-line:no-unnecessary-generics
	): CreateStyled<Props, Theme, ElementProps<Tag>>;

	// overload for component
	<Props, CustomProps>(

Resolve type incompatibilities with legacy type #722

Resolve type incompatibilities with legacy type #722

Conversation

Ailrun commented Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Ailrun Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Ailrun Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ailrun Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ailrun commented Jun 16, 2018

emmatown commented Jun 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ailrun Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ailrun Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ailrun Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

tkh44 Jun 25, 2018 • edited Loading

Choose a reason for hiding this comment

Ailrun Jun 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ailrun commented Jun 25, 2018

tkh44 commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmatown commented Jul 21, 2018

Ailrun commented Jul 27, 2018

stale bot commented Sep 25, 2018

Ailrun commented Jun 13, 2018 •

edited

Loading

Ailrun Jun 13, 2018 •

edited

Loading

Ailrun Jun 13, 2018 •

edited

Loading

Ailrun Jun 13, 2018 •

edited

Loading

Ailrun Jun 18, 2018 •

edited

Loading

Ailrun Jun 18, 2018 •

edited

Loading

Ailrun Jun 18, 2018 •

edited

Loading

tkh44 Jun 25, 2018 •

edited

Loading

Ailrun Jun 25, 2018 •

edited

Loading