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

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

Closed
mvestergaard opened this issue Jun 13, 2018 · 28 comments
Closed

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

mvestergaard opened this issue Jun 13, 2018 · 28 comments
Assignees

Comments

@mvestergaard
Copy link
Contributor

Big changes were made to the typings in the last few releases.
I tried to upgrade from 9.1.3 to 9.2.3, where it appears that inference of props types is broken.

  • emotion version: 9.2.3
  • react version: 16.4.0
  • typescript version: 2.9.1

Relevant code.

import styled from 'react-emotion';
import React from 'react';

interface MyProps {
  className?: string;
  someBehavior: boolean;
}

const MyComponent: React.SFC<MyProps> = ({className}) => <div className={className} />

const MyStyledComponent = styled(MyComponent)`
  border: ${props => props.someBehavior ? 1 : 0}px;
`;

Results in this error:

Property 'someBehavior' does not exist on type '{ theme: any; }'.
@Ailrun
Copy link
Member

Ailrun commented Jun 13, 2018

@mvestergaard I removed that inference because of withComponent.
Let's suppose that you have following code.

interface Props0 {
  typing: number;
  world: number;
}
declare const Component0: React.SFC<Props0>;
interface Props1 {
  typing: { [key: string]: string };
  safe: boolean;
}
declare const Component1: React.SFC<Props1>;

const StyledComponent0 = styled(Component0)`
  left: ${props => props.typing}px;
  top: ${props => props.world}px;
`
const StyledComponent1 = StyledComponent0.withComponent(Component1);

Now, what is the correct type for StyledComponent1? At least I cannot find the answer.

@Ailrun
Copy link
Member

Ailrun commented Jun 13, 2018

Without that inference, in other words, without dependency on props of specific component, we can determine type of props deterministic way.

@mvestergaard
Copy link
Contributor Author

So you're saying that you intend it to work this way?

Having to write this type of thing everywhere becomes tedious fast.

const MyStyledComponent = styled(MyComponent)`
  border: ${(props: MyProps) => props.someBehavior ? 1 : 0}px;
`;

Isn't the code i wrote a more common use case than .withComponent?
Should you really make that sacrifice?

This is borderline incentive for me to move back to styled-components if you're saying this is how you intend it to work.

@Ailrun
Copy link
Member

Ailrun commented Jun 13, 2018

@mvestergaard So, what's your answer? What is the correct type for StyledComponent1? Is what you mean just giving up to give correct type for components with .withComponent?

@Ailrun
Copy link
Member

Ailrun commented Jun 13, 2018

@mvestergaard Furthermore, if you use TS >= 2.9, you can extract :MyProps as a type parameter like

const MyStyledComponent = styled(MyComponent)<MyProps>`
  border: ${props => props.someBehavior ? 1 : 0}px;
`;

@mvestergaard
Copy link
Contributor Author

My answer is that you can't infer that. At best it would be Props0 & Props1
But my counter-question is, should you really sacrifice inference in the normal use case, because you can't make it work for .withComponent?

@Ailrun
Copy link
Member

Ailrun commented Jun 13, 2018

@mvestergaard And more importantly, IMO, that's not the most frequent use case, since you usually don't need to forward props for styling to internal component.

@Ailrun
Copy link
Member

Ailrun commented Jun 13, 2018

At least I use something like this.

interface MyProps {
  className?: string;
}
const MyComponent: React.SFC<MyProps> = ({className}) => <div className={className} />

interface StyledProps {
  someBehavior: boolean;
}
const MyStyledComponent = styled(MyComponent)<StyledProps>`
  border: ${props => props.someBehavior ? 1 : 0}px;
`;

@Ailrun
Copy link
Member

Ailrun commented Jun 13, 2018

@mvestergaard But, OK. If you truly want our typings to support your use case, let me have time for it. I need to think about it.

@mvestergaard
Copy link
Contributor Author

My opinion is that if possible, the code you write in typescript should not be different from what it would be in javascript. You should not need to tack on types where it can be inferred. But it seems we don't share that opinion.

The problem here is mostly that you've changed something that made existing code which worked just fine, suddenly have errors.

@mvestergaard
Copy link
Contributor Author

This is a minor thing, not related to you, but there's also the problem that the styled-components vscode extension doesn't support this syntax:

const MyStyledComponent = styled(MyComponent)<MyProps>`
  border: ${props => (props.someBehavior ? 1 : 0)}px;
`;

@Ailrun
Copy link
Member

Ailrun commented Jun 13, 2018

@mvestergaard Oh, really? Thank you for information. I didn't know that. (I don't use vscode)

@Ailrun
Copy link
Member

Ailrun commented Jun 13, 2018

@mvestergaard So, in summary, your opinion is, when there's no type parameter, props for styled component should be same with props of original component, is this right?

@mvestergaard
Copy link
Contributor Author

Yes, that was the behavior prior to the changes.

@Ailrun
Copy link
Member

Ailrun commented Jun 13, 2018

@mvestergaard I cannot find a way except breaking withComponent type (and I used it quite lot in my commercial project).
Let me ask others about this.
mitchellhamilton, tkh44, How do you think about this typing issue?
I mean, type inferencing for styled case is important, or preserving withComponent type is important? Which one is more frequent use case of emotion users?

I think I found a way. I will send a PR for this.

@Ailrun
Copy link
Member

Ailrun commented Jun 13, 2018

@mvestergaard I just opened #722.

@patrykkarny
Copy link

patrykkarny commented Jun 16, 2018

I have a similar issue in my codebase, previously was used react emotion 9.1.3 with typescript 2.8.3 and typing my styled components like this:

import styled from 'react-emotion';
import { IThemeProps } from 'types';

export interface IProps extends IThemeProps {
  fullWidth?: boolean;
  centered?: boolean;
}

export const Button = styled<IProps, 'button'>('button')`
  background-color: ${props => props.theme.color.brandPrimary};
  color: ${props => props.theme.color.black};
  min-width: ${({ fullWidth }) => (fullWidth ? 'auto' : '250px')};
  width: ${({ fullWidth }) => (fullWidth ? '100%' : 'auto')};
  margin: ${({ centered }) => (centered ? '0 auto' : 0)};
`;

and it works pretty well. Now I updated to the latest version of react-motion and typescript 2.9 and every component is complaining about improper types. Also tried the new typing for template literals in TS 2.9 but as @mvestergaard wrote above this breaks syntax highlighting in vs code. Is there any other solution to fix this?

@Ailrun
Copy link
Member

Ailrun commented Jun 16, 2018

If syntax breaking is the biggest issue, I will check the highlighting package and send a PR for updating.
Is what you guys use vscode-styled-component?

@mvestergaard
Copy link
Contributor Author

mvestergaard commented Jun 16, 2018 via email

@Ailrun
Copy link
Member

Ailrun commented Jun 16, 2018

@mvestergaard Oh, sorry. I know your biggest problem is adding type parameter itself.
Who I want to ask about it was @patrykkarny. It's my fault in my wording. Sorry for confusion.

@patrykkarny
Copy link

@Ailrun yep, I'm using vscode-styled-component, the spike of it I need to change types in almost 200 components in my codebase

@Ailrun
Copy link
Member

Ailrun commented Jun 16, 2018

@patrykkarny OK. I agreed with you that's not small change.
However, I just now notice that your problem is different from what @mvestergaard says, since what you say about is 'Emotion typings change its type parameter structure!', and what he says is 'Emotion typings now cannot infer a type parameter from its arguments!'.
Those are completely different problems in terms of typings... Could you open a new issue?

@patrykkarny
Copy link

@Ailrun your right I will open another issue for this tomorrow because it's late here

@Ailrun
Copy link
Member

Ailrun commented Jun 16, 2018

@patrykkarny However, I believe that your problem is also resolved with #722.
I know that PR is now somewhat mixed... but those changes affect each other :(

@patrykkarny
Copy link

@Ailrun #722 should fix my incompatibility issue :) so should I wait until this PR will be merged or create another issue as you suggested? :)

@Ailrun
Copy link
Member

Ailrun commented Jun 17, 2018

Please wait, what I said is my mistake. I thought I can resolve both things separately.

@patrykkarny
Copy link

It's ok :) Thanks for your work and quick answers 👍

@stale
Copy link

stale bot commented Aug 16, 2018

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.

@stale stale bot added the stale label Aug 16, 2018
@stale stale bot closed this as completed Aug 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants