glamorous-native icon indicating copy to clipboard operation
glamorous-native copied to clipboard

Optimizations/Caching

Open ssomnoremac opened this issue 7 years ago • 3 comments

Currently glamorRules like this one aren't cached:

const Row = glamorous.view(
  {
    flexDirection: "row"
  },
  (props) => ({
    justifyContent: props.centered ? "center" : "flex-start"
  }),
}

...

<Row centered/>

It seems they should if I want to use Row throughout my app. That's perhaps hundreds of uncached styles. But then if we cache styles, if we use a rule to animate a component, then we over cache by creating a cache item for each frame. Example.

(props) => {
    const {top, right, bottom, left} = props
    const isAbsolute = [top, right, bottom, left].filter(p => typeof p === "number").length > 0
    return {
      position: isAbsolute ? "absolute" : "relative",
      top,
      right,
      bottom,
      left,
    }
  }

So, just a thought. What if we both:

  1. cache glamorRules
  2. make glamorous smart enough to know if it's Animated and turn off caching for all rules

Thoughts?

ssomnoremac avatar May 18 '17 15:05 ssomnoremac

@ajwhite , what do you think if we pass a 5th arg to getStyles, isAnimated, and pass that from something that determines if the Component is Animated or not. Then we can:

//get-styles.js

function evaluateGlamorStyles(styles, props, theme, isAnimated) {
  return styles.map(style => {
    if (typeof style === 'function') {
      return isAnimated ? 
        style(props, theme) :
        StyleSheet.create({style(props, theme)}).style 
    }
    return style
  })
}

ssomnoremac avatar May 22 '17 19:05 ssomnoremac

Intersting thoughts

If we pass in isAnimated as a boolean for the getStyles call, wouldn't that end up skipping styles that might not have anything to do with an animation style?

For example:

const GlamorousAnimatedView = glamorous.animatedView(
  // static style
  {backgroundColor: 'red'}

  // dynamic, yet non animated style
  props => ({height: props.big ? 300 : 100}),

  // animated style
  props => ({opacity: props.opacity})
)

☝️ I think in these scenarios we'd want the normal behavior to happen for styles that don't contain Animated.Value entries


On another note, we will want to avoid calling StyleSheet.create if we've already created the style. I believe you had looked at some caching solutions here. Essentially we want to avoid creating StyleSheet entries with the same value over time.

atticoos avatar May 22 '17 19:05 atticoos

True, but that's not all too many uncached. Otherwise, we would need an API to identify which style rules are going to be animated and therefore not cached. I had proposed a second arg to the animated style function on which we could switch easily but I'm against that kinda thing. The API is already complicated enough and lots of people will not see it. If we do it, it should be automatic IMO. But the only way to identify animated rules, as I see it, would be tracking cache changes and disabling when changed over some arbitrary number of times.

And on your second note, I see that now. Hmm, it's harder for me to do this now that the project has been factored this way. The web project didn't have to account for this so the factoring was done without this restriction. It might just mean we have to keep styles even more split all the way through. Painful.

ssomnoremac avatar May 22 '17 20:05 ssomnoremac