[React] Add support for arrays with holes in style attribute

[vjeux]

Summary:
See the style proposal on react-future: https://github.com/reactjs/react-future/blob/master/04%20-%20Layout/Inline%20Style%20Extension.md

The goal is to enable writing CSS purely in JavaScript. We've been using this for a couple of internal projects and it's proven to be a very expressive tool.

Note that this diff doesn't bring StyleSheet.create nor StylePropType. I'm keeping it simple by just allowing arbitrary arrays, objects and falsy values.

Example of use:

...
  render: function() {
...
    <div style={[styles.base, this.state.active && styles.active]} />
...

var styles = {
  base: { color: 'red' },
  active: { fontWeight: 'bold' }
};

Test Plan:

grunt test

Reviewers: jwalke zpao sema
CC: tomocchino

[chenglou]

If this allows us to remove all the third-party react-style libraries/mixins/preprocessors then I'm all for it!
(@SanderSpies @andreypopp)

But I still think it's much more performant if you manually merge the styles outside of render and turn it into an id for className. No proof on this though.

[zpao]

The way we "corrected" style for DOM components before was easy to explain. It reflected the DOM API. This doesn't and I'm not completely sold yet that we should overload style again in a different way. We've gone from a strict type to a very loose type built into core that doesn't have a DOM corollary (at least if we ever get around to classList, that concept exists in the DOM).

This is easily extracted into a helper function (a la cx) - just pull flattenStyle out and you have to call style={flattenStyle([.........])}

[vjeux]

The goal is to move to a world where we don't use CSS anymore. Inline styles are actually a very good abstraction to replace most of CSS. I thought a lot about how to plug it into React and settled on extending the API just slightly.

@sebmarkbage has a spread operator proposal for objects but the syntax overhead is dissuasive

var style={{...styles.base, ...(this.state.active ? styles.active : {})}}

Using flattenStyle is an option but is still verbose. Also, it has a big downside that going from 1 to 2 styles requires a high cognitive overhead.

var flattenStyle = require('flattenStyle');
var style={flattenStyle([styles.base, this.state.active && styles.active])}

Inline styles are something that's used very heavily. In the biggest project where we tried this, we have 50 React components, and 100 inline styles. In another, 30 components for 130 inline styles.

[jordwalke]

But I still think it's much more performant if you manually merge the styles outside of render and turn it into an id for className.

Yes. I'd like to see a bit of discussion from @vjeux about this - I know he has ideas.
It also allows the reconciler to be much faster as you can end up comparing lists of integers.

If Paul has issue with this not being very DOM-ish, then maybe we can name it "styles" (plural)?

[jordwalke]

Using flattenStyle is an option but is still verbose. Also, it has a big downside that going from 1 to 2 styles requires a high cognitive overhead.

It also makes it harder to preflatten and perform optimizations in reconciliation.

[vjeux]

So, the idea would be to re-write

var styles = {
  base: { color: 'red' },
  active: { fontWeight: 'bold' }
};

into

var styles = StyleSheet.create({
  base: { color: 'red' },
  active: { fontWeight: 'bold' }
});

which would return a value

{ base: 1, active: 2 }

• If nothing changed, then we're just going to diff [ 1, 2 ] against [ 1, 2 ] which is going to return false and we quit.
• If something changed, then we actually flatten and pay the cost of memory allocation and copy.

This is strictly an optimization and we'll still need to support inline styles. So my intention is to first ship the easy but not so efficient version and then bring the infrastructure for the stylesheet.

[zpao]

I could be convinced, I just want to make sure we don't get too loosey-goosey. We have a DOM story and changes like this make it much harder to say no to other changes. Let's let it simmer for a few days (I'll be on PTO anyway), get some perf numbers, and hear from others before doing anything.

[vjeux]

Note that we've been building two major features with this on the mobile website. We forked React and this is an attempt to converge again.

[syranide]

which would return a value

{ base: 1, active: 2 }

If that is the goal, wouldn't we be better off by simply creating classes dynamically and returning those instead? (performance win too) As the class styles would have to be static anyway.

[andreypopp]

@syranide

The problem with CSS classes is that you need to control precedence, you want these to be different styles:

<div style={[classA, classB]}>...</div>
<div style={[classB, classA]}>...</div>

If you mix inline style with classes the former always have the priority over the later, even if it is placed before in style array:

<div style={[classA, styleB, classB]}>...</div>

styleB will override classB styles.

[sebmarkbage]

[Ugh. I thought I answered this before. I guess GitHub lost it during poor connections.]

This data model is effectively a single record and the correct data model is to create a new record. This is why we have the ... syntax. The whole point of that syntax is to enable these use cases of functional updates and to build persistent data structures into VMs and optimizers.

If the syntax isn't palatable we should come up with something new and standardize that instead. The last thing we should do is make up our own new proprietary data structure that is not generalizable. (See my JSConf.eu talk for why that is bad.)

This is a general concern with the lack of persistent immutable record types in JS which needs to be addressed at a larger scale and whatever solution we come up with should be generalizable every large immutable record type so that people don't have to come up with their own solution every time.

These data types require special methods to operate on that is specific to this field. There's no automatic conversion. Our ReactChildren data structure is bad enough. We shouldn't introduce more of them.

There's two parts to this proposal.

1. The ability to store constants as lighter representations (for example an ID but could also just be a shared pointer reference). Functional languages do these kind of optimizations all the time by treating constants as by-reference equal.

There's really no such thing as a constant because these things need to be cleaned up at some point if you have a long living app where you're able to unload modules for example. These are also not constants because they're often calculated a head of time. However, since we can assume that these objects are immutable, we can automatically or manually move declarations to the module level scope if all they depend on is branches that execute in module level scope. Thereby giving them a shared reference.

2. The second feature is the ability to combine multiple of these constants in a light-weight way without actually copying. As has already been pointed out, it's even more efficient to just do this at the module level scope. The only exception is for dynamic values such as: { ...styles.base, width: this.props.width }

These might need a persistent object. The goal should be to get these into engines and to do that we need to start designing with that in mind.

We can polyfill with an immutable record object from immutable-js or something like that. A general data structure that can later be replaced with a native record object or we can compile to support it.

As it stands right now, there's not a lot of uses of these styles on the web because it is inefficient to apply inline styles in the DOM. We would need to generate classes (the precedence is solvable).

I don't think we should introduce a new proprietary data type that will be difficult to upgrade a more general solution for persistent records. Not until there's an imminent need. There won't be an imminent need until the other aspects of this style model has proven itself. E.g. the application to DOM nodes, automatic reuse of constant values, class generation and the applicable programming model. By the time that happens, we may have the other language level optimizations in place. For example, in a compiler step.

There is a serious abstraction leak in using a non-restricted syntax for it with possible unconstrained manipulation of the arrays. That will make it difficult to minimize all the possible misuses of this feature. So it will be difficult to undo.

If we can introduce something that is more difficult to undo (such an ImmutableObject abstraction that is palatable and upgradable) then we can move forward with that instead. Or if we can prove the imminent value of this (that can't wait for other data types).

[josephsavona]

If we use StyleSheet.create({...}) and convert definitions to classNames (injecting styles into the page), then the react implementation is straightforward and plain javascript. The converted classNames can then be passed via <div className={[styles.base, state.active && styles.active]} /> and everything just works. Is there a disadvantage to StyleSheet?

[vjeux]

I'll get back to it when time is due.

[brigand]

Another concern is the ability to override styles in reusable components. We need a solid pattern for this, either the dirty practice of modifying something on the component (application level override), or a way to create a copy of a component class with modifications to the styles, or both. This is unfortunate, but important for practical uses.

With external style sheets this is less of an issue because you can just provide a default stylesheet which people can modify before using it, or they can override styles in their own stylesheet.

Somewhat common solution is to provide a renderer for certain nodes. For example:

<Form fields={fields} inputRenderer={MyInput} />

And the component would do var Input = this.props.inputRenderer || Form.inputRender || 'input';. This gets quickly out of hand for complicated reusable components.

[natew]

@brigand I've run into this and implemented something, but there are some interesting tradeoffs. I have components that use styles based on their ref key. The top-level node is always ref=self, but if a user passed in:

<Component styles={{ self: { background: 'none' } }} />

There's logic that pushes that style object to take precedence over the internal one. It works for me, but there are some interesting cases. Maybe you want to replace the styles completely and not merge them. The nice thing is I also use the keys for conditional styles, so something like:

if (this.props.active)
  this.addStyles('active') && this.addStyles('otherRef', 'otherRefActive')

Will look for the key active in the styles object and apply it to self and otherRef. But if a user passes in styles={{ active: { background: 'black' }, otherRefActive: { background: 'white' }}} those styles will be applied instead. Which gives you external themability of basically any internal style. Here's the mixin I use, far from perfect, but it's in use in production now and works pretty well.

[reverofevil]

I think the most natural solution would be CSS-like JS objects over virtual CSSOM, so that it's consistent with React's approach for HTML.

[Venryx]

I'm confused as to why React Native supports this (https://facebook.github.io/react-native/docs/style.html) but ReactJS does not.

I thought they shared the same behavior in the areas they overlap -- which would seem to be true for the "style" prop interface of components.

Or does it nowadays, and I'm just using an older version? (scared to update just to check, as last time it took some time to get the build succeeding again)

[zpao]

style doesn't map to any existing properties in react native, so there was more freedom to do something different. ReactDOM does not support arrays here yet.

…

On Thu, Feb 9, 2017 at 6:47 PM Stephen Wicklund ***@***.***> wrote: I'm confused as to why React Native supports this ( https://facebook.github.io/react-native/docs/style.html) but ReactJS does not. (I thought they shared behavior in the areas they overlap -- which the "style" prop interface is an example of) Or does it nowadays, and I'm just using an older version? (scared to update just to check, as last time it took some time to get the build succeeding again) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2196 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAg_QTeMsYbvyWZG7jR-CeTGTfjc1Wvks5ra8-4gaJpZM4CilgD> .

[sonaye]

Coming from React Native to React, having the ability to pass style arrays was a big plus.

[magrinj]

It looks like put styles in a array was enabeld in the past, but not anymore right now, is it something missed in React Fiber ?

[jsu93]

You can write a babel plugin like this https://github.com/giuseppeg/babel-plugin-classnames

And here is babel-plugin-styles from above:

https://github.com/jsu93/babel-plugin-styles/tree/styles

[giuseppeg]

Funny enough I never read this thread but I ended up implementing these ideas (except for preflattening + ids) in my library DSS https://dss-lang.com + the babel plugin above which lets you pass an array to className. Basically it is a bunch of ideas glued together - heavily inspired by React Native[ for Web] and @vjeux's talk.

You can find an example on the dss repo.

[jsu93]

@giuseppeg 👍