Glamor css prop w/ glamorous fails PropType validation

[willhoney7]

Hi there!

• glamorous version: 4.9.2
• glamor version: 2.20.40
• react version: 15.6.1

Relevant code.

import glamorous from 'glamorous';

const GlamDiv = glamorous.div({
  color: 'orange'
});

class App extends Component {
  render() {
    return (
      <div>
        <div css={{ textDecoration: 'underline' }}>glamor css prop</div>
        <GlamDiv>glamorous div</GlamDiv>
        <GlamDiv css={{ textDecoration: 'underline' }}>
          glamor css prop on glamorous div. Look at console.
        </GlamDiv>
      </div>
    );
  }
}

What you did:

I've been using glamor + glamorous and I recently discovered glamor's css prop (https://github.com/threepointone/glamor/blob/master/docs/createElement.md). It's awesome! However, I've found that it doesn't play well with glamorous.

What happened:

Reproduction:

Here's a reproduction in codesandbox: https://codesandbox.io/s/mqqq7k5z18

I also made this repo before I realized I could use codesandbox: https://github.com/Tibfib/glamor-glamorous-css-bug

Problem description:

When using both glamorous and glamor's css prop there is a PropTypes validation error. For all I can tell it works perfectly–it just complains about PropTypes.

I do think there's a use case for using both glamorous and the css prop.

Suggested solution:

Play nicely with glamor's CSS prop. I think this is a glamorous issue but it could be an issue with how glamor does the css prop.

Thanks for reading!

[marzelin]

That's rather problem with glamor/react that doesn't convert className to string but passes it as an object.
https://github.com/threepointone/glamor/blob/master/src/react.js#L17
You should file issue there (I don't see any reason why rule isn't converted to object 🤔).

[kentcdodds]

No, it's not a problem. This is a known issue. I'm on my phone so I can't explain well, but basically glamor gives back an object that has a toString method that returns the class name. There are good reasons for this.

I think our solution should be to accept an object as part of the prop-types. It's not "correct" but I don't think it's worth being correct in this situation because it causes more confusion than anything.

Want to update the prop types to accept both an object as well as a string?

[marzelin]

Having rule as an object is important in glamor so you can do this:

<El {...css()}/>

and this:

<El className={css()}>

but in glamor/react rule is assigned directly to className, so it should be a string not an object.

TIL: react doesn't check the type of className so things like:
<El className={{}}/> will create an element with class [object Object] which is probably not what you want.

Anyways glamorous can completely replace glamor/react
With glamorous you can do:

import { Div } from "glamorous"
const El = <Div css={{ textDecoration: 'underline' }}>glamor css prop</Div>

which is more flexible, ie you can do:

const ElVariant = glamorous(El)(styles)

which gives you one class with merged props. glamor/react would add two classes to an element.

[kentcdodds]

Actually, if you can use glamor/react then I recommend you do that. It'll avoid overhead from glamorous.

In any case, I've solved this issue in #333. Could use a review.

[marzelin]

Yeah, glamorous gives more flexibility but takes a bit of performance and bundle size as a price. Tradeoffs, tradeoffs everywhere :)