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.

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

Add withTheme definition #521

Merged
merged 4 commits into from Mar 1, 2017
Merged

Add withTheme definition #521

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8d5022a
Add withTheme definition
patrick91 Feb 20, 2017
da19d3d
Remove PureComponent from Component type
patrick91 Feb 28, 2017
654851b
Add better implementation of withTheme
patrick91 Feb 28, 2017
0d3f28b
Update tests
patrick91 Feb 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion native/index.d.ts
View file
Open in desktop
Expand Up @@ -11,7 +11,8 @@ export {
OuterStyledProps,
StyledFunction,
BaseStyledInterface,
css
css,
withTheme,
} from "..";

import { StyledFunction, BaseStyledInterface } from "..";
Expand Down
17 changes: 17 additions & 0 deletions typings/styled-components-hoc-test.tsx
View file
Open in desktop
@@ -0,0 +1,17 @@
import * as React from "react";

import styled from "..";
import { css, keyframes, ThemeProvider, injectGlobal, withTheme, ThemeProps } from "..";


class MyComponent extends React.Component<ThemeProps<{}>, {}> {
render() {
const { theme } = this.props;

console.log("Current theme: ", theme);

return <h1>Hello</h1>;
}
}

export default withTheme(MyComponent);
6 changes: 4 additions & 2 deletions typings/styled-components.d.ts
View file
Open in desktop
@@ -1,7 +1,7 @@
import * as React from "react";
import { StatelessComponent, ComponentClass } from "react";
import { StatelessComponent, ComponentClass, PureComponent } from "react";

type Component<P> = ComponentClass<P> | StatelessComponent<P>;
type Component<P> = ComponentClass<P> | StatelessComponent<P> | PureComponent<P, any>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think PureComponent should be here. PureComponent is an instance, but here must be a constructor.
What the use case required this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw how withRouter was implemented in React Router, they were also using PureComponent. I'll remove this :)


export interface ThemeProps<T> {
theme: T;
Expand Down Expand Up @@ -199,6 +199,8 @@ export interface ThemedStyledComponentsModule<T> {
declare const styled: StyledInterface;

export const css: ThemedCssFunction<any>;
export function withTheme<C extends Component<any>>(component: C): C;
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. it does not reflect that the resulting component does not have theme prop
  2. component parameter must have a theme prop
  3. has no strong type theming support (theme prop must be of the same type as theme)
  4. we do not need to capture component type and return it, we only need to capture props type and always return ComponentClass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. what's the point for this? you need to specify the theme prop anyway, like this:
class MyComponent extends React.Component<ThemeProps<{}>, {}> {
  render() {
    const { theme } = this.props;

    console.log("Current theme: ", theme);

    return <h1>Hello</h1>;
  }
}

export default withTheme(MyComponent);
  1. yeah, that would be helpful, will change :)
  2. same as 1
  3. will change

Copy link
Contributor

@Igorbek Igorbek Feb 20, 2017 •
edited

Choose a reason for hiding this comment

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

I'd suggest something like this here:

export function withTheme<P extends { theme?: T; }, T>(component: Component<P>): ComponentClass<rest(P, 'theme')>;

where rest(P, 'theme') is a new type operator that is going to be landed with TS smth like 2.3, unfortunately (microsoft/TypeScript#13470).
I do not have any other ideas how to express removing a property from a generic type. Do you have any?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why would you need to remove the property?

Copy link
Contributor

Choose a reason for hiding this comment

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

An inner class is required to have theme prop and an implementer may want to have it required:

interface MyComponentProps {
  theme: MyTheme; // I want have this required
  text: string;
}
class MyComponent extends React.Component<MyComponentProps, {}> {
  ...
}

<MyComponentProps theme={theme} text={text} /> // ok
<MyComponentProps text={text} /> // error, I can't use without theme

so after applying the withTheme, the component will lose theme prop:

interface MyThemedComponentProps {
  text: string;
}
const MyThemedComponent: ComponentClass<MyThemedComponentProps> = withTheme(MyComponent);

<MyThemedComponent text={text} /> // now that is ok
<MyThemedComponent theme={theme} text={text} /> // and now that is an error, because there's no theme prop anymore


export function keyframes(strings: TemplateStringsArray, ...interpolations: SimpleInterpolation[]): string;
export function injectGlobal(strings: TemplateStringsArray, ...interpolations: SimpleInterpolation[]): void;

Expand Down