Memoize `makeStyledComponent` wrapped component about react-native-zephyr HOT 5 OPEN

@nandorojo Great to hear from you! Love your work on dripsy and moti.

The classes.join(' ') in the dependency array is actually how the library used to behave. However, we added some features like dynamic classname lists and styled-components-like syntax which adds a bit of complexity to this, because technically you could pass something like ["bg:red-300", { "color:blue-100": true }] in and when you go to stringify that, you get bg:red-100 [object Object] which is a non-deterministic serialization. You can do the "flattening" outside of the useMemo, but it ended up complicating things and the memo benefits were questionable (at best).

One note: with the styled utility, the classes/darkClasses is often a stable array reference, and so memoizing on those makes sense in that context – and this whole thing of render-cycle-inline-arrays is not an issue.

The styles utility actually has caching in place, so at the end of the day these little memoization optimizations aren't that important. Passing "flex:1", "bg:red-100" multiple times is only going to result in a single style object returned, so those style object references are stable etc. With the makeStyledComponent helper, the only real "issue" I see here is the fact that those inline arrays could trigger re-renders even if nothing really changed between renders. A somewhat simple comparison function used in React.memo would probably suffice to make that a non-issue.

As for your comment about the babel plugin! That's been on my mind, too. If StyleSheet.create actually ever implements any sort of perf optimization, then I'd be much more tempted to write a babel plugin to extract things out to a StyleSheet.create call at build time. But I totally agree – I don't think the added complexity is worth it here, since there isn't really any perf benefit. The approach of generating styles on the fly also has the "theoretical" benefit of not having to have a ton of style objects shipped in the app bundle – if the user never visits the deeply-nested settings screen in the app, the style objects on that screen don't ever have to be created.

Thanks again for the feedback and checking this library out! Please do reach out if you have any other thoughts, love to hear any sort of feedback!

from react-native-zephyr.

I'm not sure the right way to profile this, but one simple option would be to edit the useMemo dependency:

Rather than this:

return React.useMemo(() => {
  const allClasses = [...classes].concat(isDarkMode ? darkClasses : [])
  return styles(...allClasses)
}, [classes, darkClasses, isDarkMode])

The classes in the dependency array could be .join'd:

return React.useMemo(() => {
  const allClasses = [...classes].concat(isDarkMode ? darkClasses : [])
  return styles(...allClasses)
}, [classes.join(' '), darkClasses, isDarkMode])

It's always hard to know the role of memoization for something like this. In dripsy, I use a stable memo hook. But since this library only uses an array, that would overkill.

An alternative would be for a compiler to turn the classes array into a string or even into StyleSheet.create objects at build-time. This is what a few libraries like NativeWind are doing now, and a simple array of strings lends itself well to this. That said, requiring users to add a babel plugin is a bit too magical for my taste and adds complexity to the maintainers.

Figured I would share my thoughts. Great work to the creators here!

from react-native-zephyr.

thanks for the thoughtful reply!

i have a few thoughts from what you said here, but i'm in the middle of work now so i'll save most of them for later.

one thing worth mentioning is that StyleSheet.create does have perf benefits on Web. RNW 0.18 also no longer adds vendor prefixes to inline styles to discourage their usage.

the babel plug-in's main benefit would probably be getting rid of unnecessary work at runtime (which is always theoretically attractive). Even if it just creates plain objects outside of render (which is what StyleSheet does on iOS/Android) it would probably be better than parsing styles during render.

if the user never visits the deeply-nested settings screen in the app, the style objects on that screen don't ever have to be created.

agreed. in addition to my bias towards babel plugins adding complexity / magic, this was my thinking with dripsy as well. nothing is free here.

The styles utility actually has caching in place, so at the end of the day these little memoization optimizations aren't that important. Passing "flex:1", "bg:red-100" multiple times is only going to result in a single style object returned, so those style object references are stable etc.

i also did this originally. however i was concerned that i may be overly caching globally and messing up some sort of garbage collection stuff so i stopped doing it. but since you're caching a fixed set of string rules, it probably makes more sense. in dripsy's case, i was caching any arbitrary rule used, which felt wrong (keyword: "felt" lol).

finally: on the Web side, i think RNW is already doing this level of caching for you. not sure to what extent you're targeting Web, but I find RNW's new approach to this stuff interesting.

btw, have you seen nativewind?

https://github.com/marklawlor/tailwindcss-react-native

on Web, it's using pure CSS class names, thanks to the new RNW API that works with styleq under the hood. thought it might be of interest.

guess i couldn't resist sharing my thoughts now haha

from react-native-zephyr.

These are all interesting points! TBH I haven't thought much about RNW, my head's been mostly in the iOS/Android space. But this is all really good to know, since RNW support should be a goal of the project, too.

I haven't checked out NativeWind in depth, I'll have to take a more thorough peak. Playing with it now, it looks pretty awesome. I'll have to give it a spin to see how it compares to Zephyr.

from react-native-zephyr.

that all makes sense, sounds good! excited to see what you guys continue to build here.

from react-native-zephyr.