[typescript] withStyles() complains unless using as

[franklixuefei]

• I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

No type errors should be reported for adding legal styles kv pairs in withStyles()

Current Behavior

For some style properties, like position, overflow, etc, specifying a valid value, e.g., position: 'absolute' or overflow: 'auto' reports type errors.
Example:

The only hack is to use as:

Your Environment

Tech	Version
Material-UI	beta.41
React	16.2.0
etc

[estaub]

This is a result of the recent updates to @types/jss, at https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/jss.
AFAICT, no one's raised an issue on it there yet; someone from mui definitely should!

They (the folks working on the jss type defs) are doing some very radical changes without a good rollout process, imho. It's not clear to me what a better process would be, though; maybe someone else has a thought.

(I'm not a mui project member or anything, I'm just sharing what I know/think.)

[franklixuefei]

@estaub Thanks for your reply! I've opened an issue there - DefinitelyTyped/DefinitelyTyped#24952

[pelotom]

These issues have existed for a long time; the core issue is TypeScript’s type widening. They are just more prominent now that JSS has better typings. See #8928 for techniques to guide the type system.

@estaub I’m one of the people pushing the radical changes. What would you suggest as a better rollout process?

[estaub]

@pelotom
As I said, I don't have a brilliant idea; I'm afraid I'm being more critical than constructive!
For more, let's move this over to DefinitelyTyped/jss, where others are more likely to see and contribute. See DefinitelyTyped/DefinitelyTyped#24955.

[appsforartists]

Another anecdote re: breaking changes:

A few months ago, JSS typings didn't exist. There were a few rumblings on the issue tracker, but no one had taken the onus to write them (and someone was squatting on @types/jss for an unrelated project with ~0 users).

So, I spent an afternoon: threw together some basic types and got them merged into DT. The only interesting thing about them was the use of keyof to give completions on class names. I didn't expect much of it, and tbh I don't even use JSS that often.

Apparently, they're massively popular. That popularity also brought folks like @pelotom who has taken the time to make them super precise.

So, there's no shadowy cabal of folks trying to "move fast and break [typings]." We don't even know each other. We have just independently tapped into an unmet need of providing typings for this library.

I understand that changes can be painful. Part of the reason there's no roadmap is because there's no team to coordinate. The changes come from anyone who happened to feel like making them and opening a PR.

[franklixuefei]

@pelotom I agree that type widening sucks in many scenarios. However, I think there's a way to disable it, but that requires some type definition changes. See this - https://blog.mariusschulz.com/2017/02/04/typescript-2-1-literal-type-widening#non-widening-literal-types

[pelotom]

@franklixuefei I don't see anything in that link that suggests a way to change the type definitions to solve this problem. But if you have specific suggestions I'm all ears!

I made a proposal a while back to solve this problem, and others have been made as well. I don't know what to suggest except comment on those issues, or create new proposals.

[franklixuefei]

@pelotom In his post, he mentioned that

You can create a variable of a non-widening literal type by explicitly annotating the variable to be of a literal type:
const stringLiteral: "https" = "https"; // Type "https" (non-widening)
const numericLiteral: 42 = 42; // Type 42 (non-widening)

I guess in order to achieve this, React.CSSProperties needs to change, for example position?: 'absolute' | ... into position?: 'absolute' | ... = 'absolute' | ..., which we don't have control over.

And I just went over and read about your post under Typescript, which is great! I hope this can raise their attention a bit. We really need a flag in the compiler options to turn it off.

[pelotom]

I guess in order to achieve this, React.CSSProperties needs to change, for example position?: 'absolute' | ... into position?: 'absolute' | ... = 'absolute' | ..., which we don't have control over.

I think you may be getting confused between interfaces and object literals.

[estaub]

For other possibilities and a workaround, see DefinitelyTyped/DefinitelyTyped#24952 (comment)

[varoot]

I avoid this error by declaring type on the argument being passed to withStyles

import { StyleRulesCallback, withStyles } from 'material-ui/styles';

const styles: StyleRulesCallback<'root'> = () => ({
  root: {
    position: 'relative',
  },
});
const decorate = withStyles(styles);

[estaub]

@varoot if you have a lot of them, you may find this less tedious:

Somewhere, once:

export function makeTypeCheckProperties<MemberType>():
    <S extends {[key in K]:MemberType}, K extends keyof S>(ss: S) => S {
        return s=>s
}
export const checkSheet = makeTypeCheckProperties<CSSProperties>()

Then I follow a rule of thumb that may or may not be worth following for you:

Always wrap the braces around a literal stylesheet in either withStyles or checkSheet.

This helps with some other scenarios, too, like using ... to include other stylesheets that contain e.g. position:'relative'. So in this case, following the rule:

const styles = () => (checkSheet({
    root: {
        position: 'relative',
    },
}));
const decorate = withStyles(styles);

I'd be grateful for feedback from anyone who tries this.

[pelotom]

There was a proposal once upon a time to add something like this (albeit at a more granular level) to the library:

#8819

But it was shut down. Maybe worth reviving?

[estaub]

@pelotom I actually started to write a PR a few days ago, but got PR fatigue ;-). If you want to run with it, please do! Otherwise, I'll be back with it in a few days/weeks.

The checkSheet sheet-wrapper approach in my comment above is much easier to use than the style-wrapper one offered in #8819, IMHO. It's a re-implementation of the defective one posted in DefinitelyTyped/DefinitelyTyped#24952 (comment) .

[oliviertassinari]

Apparently, they're massively popular. That popularity also brought folks like @pelotom who has taken the time to make them super precise.

@appsforartists as far as I know. It was the other way around. @pelotom pushed for adding the projet as a dependency of Material-UI.

[pelotom]

This should be resolved by #11609, take a look at the updated TypeScript guide there and feel free to comment.

[jeffshaver]

@pelotom can't wait for these changes to land in a release! Thanks!