discussion - remove spread syntax
Closed this issue Β· 23 comments
I'm considering removing the data attribute method of adding styles. ie -
<div {...rule}/>preferring the plain class syntax instead
<div className={rule}/>thoughts?
pros - faster object/rule creation, smaller css payload
cons - not backward compatible, no debug labels
// cc @KyleAMathews @kentcdodds
(to be clear, am thinking of other ways to get readable classnames, just not as part of this discussion)
glamorous prefers, uses, and optimizes for className. I've used the spread thing and liked that a lot, but for most people I think this is more confusing than anything. Using className is more straightforward I think. I'm π on this change.
We only use the spread syntax at our company and we like it a lot. It was always easier for composition, so we don't have to concat class name strings π€
Would this break the glamor/jsxstyle ?
@jaredpalmer no, should be just fine.
We use and like the data attribute. But for an performance increase happy to switch. Maybe it would be nice to provide a code of to the CSS prop?
I use and like the data attribute approach but would be interested in having only a single way to apply and target styles. classnames are the standard so that seems like a win over the data-attributes if we're consolidating. As a bonus it means that the bailout option of targeting classes inside of a component is mentally easier to model:
// imagine "gStyle" is imported and <li> is a self-contained component
const gStyle = css({ backgroundColor: 'red' });
const overwriteStyle = css({
[`& .${gStyle}`]: {background: 'pink'}
})
class App extends Component {
render() {
return (
<ul className={overwriteStyle}>
<li className={gStyle}>
This is red.
</li>
</ul>
)
}
}<style type="text/css" data-glamor="">
.css-c8s6xq,[data-css-c8s6xq]{background-color:red;}
/* don't have to worry about `[data-*] [data-*]` combinations anymore */
.css-1j9r7jh .css-c8s6xq,[data-css-1j9r7jh] .css-c8s6xq{background:pink;}
</style><ul data-reactroot="" class="css-1j9r7jh">
<li class="css-c8s6xq">This is red.</li>
</ul>Was that before or after css() became more capable? Would you be willing to
use a custom glamor plugin ? Maybe a custom createElement?
I think it was before css(), yes.
We try to avoid framework specific Babel plugins, because we share our build config within multiple projects at our company (similar to create-react-app, but without eject feature, because we don't want someone to not use our build config). Only if they offer a lot of value or because there is no workaround we add something like that.
I think the spread syntax helped us to adopt Glamor internally. AFAIK it helped our designers to build a different mental model so that they have to rethought their best practices from vanilla CSS to use CSS-in-JS.
We use propMerge for React components. I guess we'd need something for class names which works similar. (AFAIK glamorous also does this internally.)
If we don't need/want spread syntax anymore we should discuss, if we just return the string of the class name instead of an object, so we don't have to think about #193.
I dunno... what do you think @otbe?
we'll have to always return the object, else the weakmap caches can't be used (which is fairly unique to glamor, and a huge reason it's usually fast).
this is just a discussion, so we might not do this at all. I'm personally tending towards doing it, but happy to listen for now.
Dunno.
pros - faster object/rule creation, smaller css payload
I guess smaller css payload because we can remove the selectors for data attrs, but whats the reason behind faster object/rule creation? Glamor is very fast. We have no problems in some bigger SPAs at work. I think its hard to sell to POs to change a lot of code for a small performance gain, because like @donaldpipowitch said we use spread syntax all over our codebase :)
Nevertheless we can go for class/className instead of data selectors. Maybe something like this?
const rule = css({ backgroundColor: 'green' })
<div {...rule}/>
/// equals
<div className={rule.className}/>
(would be only a soft breaking api change :D)
In addition to @otbe:
For easy cases:
const rule = css({ backgroundColor: 'green' });
const Comp = () => <div {...rule}/>;
// equals: const Comp = () => <div className={rule.className}/>;
For more complex cases:
const rule = css({ backgroundColor: 'green' });
const Comp = ({ className, ...props }) => <div {...rule.merge(className)} {...props}/>;
// equals: const Comp = ({ className, ...props }) => <div className={`${rule.className}${className ? ` ${className}` : ''}`} {...props}/>
Or something like that?
{className: ''} would break with multiple rules on the same element. bit too icky for me.
faster object/rule creation
Every rule created by glamor has a unique hidden class, because they all have unique keys (data-css-*). By removing that key, all rules will share the same 'shape', making rule creation faster overall. This comes into play when you have many (ie: thousands) of unique rules. Maybe not such a big deal.
@threepointone why not remove the spread operator as default behavior and give an optional plugin? With the way the plugins are working it would be just a few lines of code :)
IMHO the syntax is weird and I would rather not have to clutter my CSS if I'm not using it.
EDIT: I think it would be cleaner that css() returns the className as string. Anyone using the spread operator could use a plugin and a helper to spread the value.
@threepointone I'm still having to use "toString()".
I would like to use spread over "toString" but I'm worried that spread will become depreciated in this library.
Are you looking to implement coercion for this in an upcoming version?
I expect I'd spread was deprecated, then you wouldn't have to use toString anymore.
going to keep the spread form. closing this.
@threepointone +1 for making the spread optional. would love to reduce our SSR size
The good news is there are plans afoot for making this configurable, can't promise a timeline tho.
awesome! π