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

ReactJSS - Incompatible types error (CSS properties for alignment) #1258

Closed
AlicanAkyuz opened this issue Jan 8, 2020 · 8 comments
Closed

Comments

@AlicanAkyuz
Copy link

Expected behavior:
When a CSS property for alignment is used (such as textAlign and flexDirection), the type definition of the property should match and we should not get an error.

Describe the bug:
When a styles object is passed to createUseStyles, we get the following error in v10.0.3. The error is thrown only when the styles object has a className key that includes a CSS property for alignment, such as -> textAlign: 'right'

71966876-a8b55180-3202-11ea-84ce-af6eacd57212

Example
SomeComponent:
Screenshot 2020-01-08 at 17 19 10

SomeComponent.styles
Screenshot 2020-01-08 at 17 19 31

Has anyone experienced this and found a way to work around? It works unproblematically when downgraded to v10.0.2

Versions (please complete the following information):

  • jss: 10.0.3
  • Browser [chrome]:
  • OS [macOS]:
@kof kof added the typescript label Jan 8, 2020
@kof
Copy link
Member

kof commented Jan 8, 2020

I assume this is a typescript issue.

@mattwagl
Copy link

I'm also experiencing similar issues with the new typings 10.0.3. In the last few days I've conducted several small experiments in order to track down what works and what doesn't. Some of the insights might be helpful.

These errors seem to happen when type inference of TypeScript somehow fails. It only happens for CSS properties where values are typed as union types of different string values. Like textAlign, or position.

I managed to get rid of them when explicitly typing the styles using the Styles type. I've created a CodeSandbox:
https://codesandbox.io/s/blissful-raman-i6ppw

It works when using explicitlyTypedStyles and it fails when passing untypedStyles to createUseStyles.

@HenriBeck
Copy link
Member

This seems to be a common issue, Material UI also has that issue. We could widen all of the csstype properties to also allow string, but then you would lose some validation of the styles of course.

@mattwagl
Copy link

@HenriBeck Thanks for the clarification! I think it's great to have these strongly typed CSS objects. So widening these properties is not necessary. It's just important to use them in the intended way.

That's why I think what's currently missing is a page in the docs that illustrates how to correctly use TypeScript with jss and react-jss, for both static and themed styles. This could prevent future issues like this one. And it's something to link to when new TypeScript issues are being filed in the future. At the moment we're trying to guess the correct usage from the typings files.

I can offer to create a PR if this helps. I would just need some help on how to contribute and on how the types should actually be used. For example for the dynamic (themed) use case, I'm still trying to figure out if this would be the correct setup: https://codesandbox.io/s/boring-fermi-qeicu I think I figured out from other issues that it's possible to extract the classNames, even for this dynamic use case: https://codesandbox.io/s/interesting-microservice-z03ny which was an interesting learning for me. Sadly there's one case where this doesn't work: when you create keys for the styles object on the fly. For example we've got helper functions to generate media queries like this: https://codesandbox.io/s/modest-monad-h9jvt

In any case: thanks for creating this library and for pushing things forward. :-)

@HenriBeck
Copy link
Member

Regarding the second code sandbox, I will have a look at why the inferring of keys doesn't work there.

The last codesandbox can be fixed by explicitly typing the getMediaQuery as that is currently inferred as () => string.

Feel free to create a PR regarding the DOCS or I can make some time next weekend for this

@mattwagl
Copy link

@HenriBeck I did some additional experiments and thought I'll share my insights.

I think type extraction (using the keyof ReturnType pattern) for styles created by a function only seems to work when the getStyles function is a lambda function that immediately returns an object literal. Otherwise TypeScript cannot extract a proper type definition that is handed over to the ReturnType type. It's not easy to find some TypeScript documentation for this.

So that currently leaves us with two ways to type dynamic styles.

Variant A: Explicitly typed literal types and extracted class names

import React, { FunctionComponent } from 'react';
import { render } from 'react-dom';
import { createUseStyles, Styles, ThemeProvider } from 'react-jss';

class Theme {
  public colorForeground: string;
  public colorBackground: string;

  constructor() {
    this.colorBackground = 'red';
    this.colorForeground = 'white';
  }
}

const getStyles = (theme: Theme) => ({
  '@global': {
    '*': {
      padding: 0,
      margin: 0
    }
  },

  MyComponent: {
    color: theme.colorForeground,
    background: theme.colorBackground,
    textAlign: 'left' as 'left'
  }
});

const useStyles = createUseStyles<keyof ReturnType<typeof getStyles>>(
  getStyles
);

const MyComponent: FunctionComponent = () => {
  const classes = useStyles();

  return (
    <div className={classes.MyComponent}>This component is styled.</div>
  )
};

const App: FunctionComponent = () => {
  const theme = new Theme();

  return (
    <ThemeProvider theme={ theme}>
      <MyComponent />
    </ThemeProvider>
  )
};

const rootElement = document.getElementById('root');

render(<App />, rootElement);

A big downside of this variant is that you need to prevent "Incompatible types" errors for CSS properties typed as literal types on every occurrence since you cannot type the whole return value as Styles. Even worse is the fact that these errors surface when createUseStyles is called. So it's very hard to track which properties actually caused the problem. So that brings us to…

Variant B: Explicitly typed return Values and hard coded class names

//...
type MyComponentClasses = 'MyComponent';

const getStyles = (theme: Theme): Styles => ({
  '@global': {
    '*': {
      padding: 0,
      margin: 0
    }
  },

  MyComponent: {
    color: theme.colorForeground,
    background: theme.colorBackground,
    textAlign: 'left'
  }
});

const useStyles = createUseStyles<MyComponentClasses>(
  getStyles
);
//...

In this variant you loose the ability to extract the class names as you explicitly type the return value of the getStyles function. So one needs to duplicate the class names. But the big advantage here is that you get errors when using false CSS values pretty close to where they are defined. So it's my current favorite for defining themed styles.

Creating a PR to the docs

When looking at the docs website I think a page Using TypeScript under the Advanced Guides would be a good place for this kind of documentation. But I'm not quite sure where to start with this. There's a docs folder with markdown files that seem to get rendered using this repo. So I'm a bit unsure where I would need to define things. And there's a huge number of branches in this repo. Which one would be a good starting point for a new branch?

One last thing

You wrote …

The last codesandbox can be fixed by explicitly typing the getMediaQuery as that is currently inferred as () => string.

But which type should one return there? Sorry for this silly question but I'm still trying to get my head around TypeScript. :-)

@pajasevi
Copy link

This is related to #1255

@kof
Copy link
Member

kof commented Jan 28, 2020

fixed in 10.0.4

@kof kof closed this as completed Jan 28, 2020
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

5 participants