-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
Question regarding latest (beta.43) WithStyles update. #11109
Comments
@emreeren No, it was very much not intentional! @pelotom Reversing the order of the alternative function types returned by withStyles seems to fix this particular case. Do you recall if you had a reason (test case) for the original order? export default function withStyles<ClassKey extends string>(
style: StyleRules<ClassKey> | StyleRulesCallback<ClassKey>,
options?: WithStylesOptions,
): {
<P extends ConsistentWith<WithStyles<ClassKey>>>(
component: React.ComponentType<P & WithStyles<ClassKey>>,
): React.ComponentType<P & StyledComponentProps<ClassKey>>;
(component: React.ComponentType<WithStyles<ClassKey>>): React.ComponentType<
StyledComponentProps<ClassKey>
>;
}; |
Nope, reversing the order seems like the right move as long as it satisfies the rest of our test cases. We should add this example as a test case too. |
@emreeren The bug does not occur if
is set in tsconfig.json. @pelotom Swapping the order of the function-overload alternatives now fails where one might suspect: the no-props case. I've no idea of how to fix the reversed case. It seems over-presumptive to me, but I need to ask: is it reasonable to require |
I think it's reasonable to expect that people use |
Sounds good. Unrelated but at my first attempt I've got an issue because of this. ReactiveX/rxjs#3031 |
@ljvanschie Does changing to |
@estaub I've tried the setting, but it caused some other problems with stateless functional components that use |
@ljvanschie if it caused problems, you should probably pay attention to them... it’s helping you find bugs! |
I ran into the same problem as OP, enabling |
Actually, enabling |
I looked into this some more and I don't think this has anything to do with type ComponentProps = IComponentProps & Partial<WithStyles<'content'>>; Since |
Actually, it should really be type ComponentProps = IComponentProps & StyledComponentProps<'content'>; but the typings are subtly wrong to make that not work... I will fix that. |
The point is that the inner |
@geirsagberg I'm not sure what you mean... |
Nevermind, I think I misunderstood. So the So if you set
|
Yep, that's the idea! |
Hm, I misspoke though, you can't fix it by just saying type ComponentProps = IComponentProps & Partial<WithStyles<'content'>>; and interface IComponentProps {
caption: string;
}
const decorate = withStyles<'content'>(theme => ({
content: {
margin: 4
}
}));
const StyledComponent = decorate<IComponentProps>(props =>
<div className={props.classes.content}>Hello {props.caption}</div>
);
<StyledComponent caption="Developer" /> |
Wasn't the point of #11003 to avoid the necessity of an extra generic parameter though? Or what were the other benefits? |
Yes, that was the point of it... however it appears that benefit was predicated on using |
Ok, let me explain a bit. The reason it fails without It reads (component: React.ComponentType<WithStyles<ClassKey>>): React.ComponentType<
StyledComponentProps<ClassKey>
>; and question is whether the parameter declaration in usage (props: IComponentProps & WithStyles<'content'>) => {
return <div>...</div>
} matches the withStyles declaration's parameter component: React.ComponentType<WithStyles<ClassKey>> With bivariance (without
which of course is true, but not the right question. With contravariance (with
which is false, and what we want. This may seem backward, until you think about it in the context of being a function argument, which is exactly what
With |
@estaub nice analysis, that makes sense. So I think the question is, is there a way we can disqualify the first alternative even when |
This may also be something that could be handled via conditional types instead of overloads, but I’m pretty sure requiring TS 2.8 as a baseline would be more disruptive than requiring |
@estaub There is a problem with a static stylesheet as well: This sample works only if I change L25 to |
@pelotom I've thought on it a lot without getting any traction, but I'm no Typescript maven. I very much identify with developers tired of definition upgrades breaking their code all the time. (Witness my rantiness about earlier csstype-related issues.) But we're hardly alone; redux just did a big redefinition churn. Flipping the two overload alternatives might work if there was some way to restrict P to cases where If you recall, I found an apparently solid implementation that got tripped up by scenarios where the inner props are a discriminated union. FWIW, I have deep misgivings about the continued viability of a union as a top-level prop definition; I'm pretty sure at least some other HOC implementations (e.g. react-router's) run afoul of them also. So I'd trade them away quickly; if someone wants a discriminated union it has to be wrapped in an object. Obviously, everyone's mileage will differ; I suspect the union-based test case and docs arose from some real usage. Even when it's viable to require 2.8, I don't believe 2.8's conditional types will help with this, again, as long as support of a top-level union prop definition is required. This is based on experimentation. |
You’ll be happy to learn I was responsible for that as well 😜 reduxjs/redux#2563 |
@geirsagberg Yes, thanks, I see it now; my thinking that this is dependent on the |
The workaround const StyledComponent = decorate<IComponentProps>(Component); gives me qualms, because it requires specification of the wrong generic; when inference is working properly, the inferred parameter is Here are a couple of constructs, either of which, if it existed, would allow a declaration that supports
Update |
It's not wrong, it's minimal by design. You shouldn't be required to specify redundant information, i.e. the type of const StyledComponent = decorate<{ content: string }>(({ content, classes })=> (
// `content` and `classes` are inferred correctly
<div className={classes.root}>{content}</div>
)); |
FWIW, the problems with support of unions in other implementations arises out of the definition of interface A { d: 'A', a: string }
interface B { d: 'B', b: string }
type Key = keyof (A | B)
const key: Key = 'a' // TS2322: Type '"a"' is not assignable to type '"d"'. Discussion: microsoft/TypeScript#12948 et al. |
Thank you very much for the detailed explanation of the case. However I think old type declaration should just work fine without need of an alternate declaration. <P>(component: React.ComponentType<P & WithStyles<ClassKey>>): React.ComponentType<P & StyledComponentProps<ClassKey>>; On the test case |
@emreeren
I'd call it a deficiency that was fixed by Don't take either of the above as an opinion about what we should do going forward. I'm still churning, trying to find something that works well in all cases, including without |
I'm using 2.8.3 and please let me know if you have something to try. |
I have a solution which solves all known edge cases, but requires TypeScript 2.8. See #11280, and feel free to comment there. I'd like to get a sense from the users whether 2.8 is an acceptable baseline. |
I'm still running into this issue on the latest release and typescript 2.8.3. I'm super new to typescript so I'm probably missing something - is there a way the code needs to be written to work with the merged fix? I'm using this method to define props from the docs:
|
@kevinhughes27 can you provide a complete self-contained example? |
Sure: import * as React from 'react';
import * as ReactDOM from 'react-dom';
import AppBar from '@material-ui/core/AppBar';
import Toolbar from '@material-ui/core/Toolbar';
import Typography from '@material-ui/core/Typography';
import IconButton from '@material-ui/core/IconButton';
import MenuIcon from '@material-ui/icons/Menu';
import { withStyles, WithStyles } from '@material-ui/core/styles';
const styles = {
flex: {
flex: 1,
},
menuButton: {
marginLeft: -12,
marginRight: 20,
},
};
interface Props extends WithStyles<typeof styles> {
openNav: (event: React.SyntheticEvent<{}>) => void,
}
class TopBar extends React.Component<Props> {
public render () {
const { classes } = this.props;
return (
<AppBar position="static">
<Toolbar>
<IconButton className={classes.menuButton} color="inherit" aria-label="Menu">
<MenuIcon />
</IconButton>
<Typography variant="title" color="inherit" className={classes.flex}>
Title
</Typography>
</Toolbar>
</AppBar>
);
}
}
const StyledTopBar = withStyles(styles)<{}>(TopBar);
class App extends React.Component {
public render () {
return (
<div>
<StyledTopBar openNav={() => { return }} />
</div>
);
}
}
ReactDOM.render(
<App />,
document.getElementById('root') as HTMLElement
); fails with:
|
@kevinhughes27 try removing the -const StyledTopBar = withStyles(styles)<{}>(TopBar);
+const StyledTopBar = withStyles(styles)(TopBar); this annotation used to be required to make type inference work out, but is no longer necessary, and in fact creates spurious errors. |
That did it! The example app still has the old syntax. I can submit a PR later. I think a couple of other things in the example are outdated as well. |
Expected Behavior
Sample code to compile.
Current Behavior
does not compile with that error
Steps to Reproduce (for bugs)
Context
The sample code was compiling fine before beta.43 update. Now I need to change
line as
to make it compile without errors.
I'm not sure if it is a bug or an intentional change so I wanted to ask before attempting to change my all styled components. I think this probably relates with #11003
Your Environment
The text was updated successfully, but these errors were encountered: