FormidableLabs / radium

A toolchain for React component styling.
http://formidable.com/open-source/radium/
MIT License
7.39k stars 308 forks source link

Alternate syntax for higher performance #512

Open ianobermiller opened 8 years ago

ianobermiller commented 8 years ago

Instead of traversing the tree of rendered elements, we might consider an alternative that is a bit more invasive , but should be much faster. We create wrapper components with the same name as built-in elements, each of which knows how to handle the style prop in The Radium Way. Example usage:

import {Div} from 'radium-builtins';

class App extends Component {
  render() {
    return (
      <Div style={{':hover': {color: 'blue'}}}>Hover me!</Div>
    );
  }
}

For reference, the equivalent in today's Radium:

@Radium
class App extends Component {
  render() {
    return (
      <div style={{':hover': {color: 'blue'}}}>Hover me!</div>
    );
  }
}
gaearon commented 8 years ago

What about when two libraries want to do this? Like Animated.div.

ianobermiller commented 8 years ago

Yeah, that did cross my mind, and I don't know if I have a good answer for it yet. It could be that in addition to exporting Radium.Div, it exports Radium.wrap, which you could wrap around 'div' or another component, like Animated.div. I'd like to see this kind of extensibility built into the framework.

arbesfeld commented 8 years ago

Could this be done at build-time? An idea: if a JSX element has an attribute radium={true} then it is wrapped in a Radium.Div.

ianobermiller commented 8 years ago

@arbesfeld It wouldn't be hard to write Babel plugin to do so. Might as well just add it to every element, and not even bother with the attribute noise. I'd prefer to keep Radium 100% runtime only, though.

bobbyrenwick commented 8 years ago

:+1: - this sounds really good - we're getting to the point in production where Radium is becoming an issue with speed of rendering. Doing this instead of going through each of the children sounds really sensible!

ianobermiller commented 8 years ago

we're getting to the point in production where Radium is becoming an issue with speed of rendering

@bobbyrenwick Could you elaborate here? I'd love to get more information about your use case and figure out if we can speed it up at all without this work, which is longer term. How did you determine Radium was a bottleneck? Are you using shouldComponentUpdate in strategic places? Are your components properly decomposed (no huge components with many elements, each with interactive styles)?

bobbyrenwick commented 8 years ago

hey @ianobermiller sorry for the tardy response. I've not forgotten about this, I just haven't had the time to give you a proper answer!

In short, in large list views, where each item is quite complicated, resolve-styles takes around 30% of render time. I'm sure that some of this could be improved by breaking down the components into smaller pieces but haven't had an opportunity to test that idea out yet.

ianobermiller commented 8 years ago

@bobbyrenwick thanks for some numbers, we should definitely put together some kind of test that exemplifies the problem and then look into perf optimizations.