akveo / react-native-ui-kitten

:boom: React Native UI Library based on Eva Design System :new_moon_with_face::sparkles:Dark Mode
https://akveo.github.io/react-native-ui-kitten/
MIT License
10.19k stars 952 forks source link

Could UI Kitten use StyleSheet.create to generate classes instead of inline styles on web? #1266

Open sschottler opened 3 years ago

sschottler commented 3 years ago

πŸ’¬ Question

Could UI Kitten use StyleSheet.create to generate the style objects inside the render methods when new eva styles are received from the higher order styled component?

If so, expo web/react-native-web would generate atomic css classes instead of redundant inline styles, which is what currently happens because UI kitten does not use the StyleSheet.create api to generate the style objects that get set as the style prop on react-native primitives (inspect any example from docs to see inline styles). This is suboptimal for web output, because if you have 20 UI kitten buttons on a page, you get all the styles repeated inline 20 times. If the StyleSheet.create api was used, all 20 would reference the same atomic css classes for each style rule. However, there may be performance implications of using StyleSheet.create in render functions that I'm not considering, so that's why I submitted a Question instead of a Feature Proposal.

Example

This generates inline styles on react-native-web:

const CustomElement = (props) => {
  const container = {
    backgroundColor: "green",
    width: 200,
    height: 200,
    color: "#fff",
  };
  return <View style={container}>some element</View>;
};

Output:

inlinestyles

This generates atomic css classes because it uses the StyleSheet.create api:

const CustomElement = (props) => {
  const styles = StyleSheet.create({
    container: {
      backgroundColor: "green",
      width: 200,
      height: 200,
      color: "#fff",
    },
  });
  const { container } = styles;
  return <View style={container}>some element</View>;
};

Output:

atomiccss

Code Change

I believe making this change would involve updating all the getComponentStyle methods to call StyleSheet.create instead of returning a regular object:

https://github.com/akveo/react-native-ui-kitten/blob/cacd67cb5ac0303eb9179e24607e695b58f012dd/src/components/ui/button/button.component.tsx#L167-L183

UI Kitten and Eva version

Package Version
@eva-design/eva 2.0.0
@ui-kitten/components 5.0.0
RWOverdijk commented 3 years ago

@sschottler a bit late perhaps, but I too have this question. I think the challenge with this is the dynamic nature of these classes. UI-Kitten works on web, but wasn't designed for web. Some of the architectural decisions reflect this.

To get this to work, each "style" that is generated (as in, each style passed through the styled() hoc) would have to be passed through StyleSheet.create(). This gets tricky since some of these styles are calculated beforehand (it returns one object that has been normalised instead of multiple that override each other). On top of that, every component that requires styles would need to be its own eva-design based component or its own StyleSheet.create() call to make sure it passes through StyleSheet.create().

Also afaik things like animations won't work as they require the component reference to do their thing. Each component would have its own animation that can't be created through StyleSheet.create().

That being said, I do think this would be a good change. I personally don't see a problem with this approach other than it being a breaking change and breaking composability on the styles returned by the hoc.

It would be a lot of work though. I think some big steps could be made by changing this into a mechanism that somehow caches styles. The issue I see here is that it needs to keep track of the theme as well.

TL;DR;

This can be done and I think it should. But I also think it should be done incrementally (create when not created yet) as it would otherwise be a pretty big hit on initial performance. In any case this means creating stylesheets for every individual variantGroup and appearance which is no small task.

sschottler commented 3 years ago

After reading your comment, I was curious what happens if you move StyleSheet.create calls inside a render function on react-native-web so I manually modified node_modules/@ui-kitten/components/ui/button/button.component.js and wrapped this.getComponentStyle(eva.style) call in StyleSheet.create call:

- const evaStyle = this.getComponentStyle(eva.style);
+ const evaStyle = react_native_1.StyleSheet.create(this.getComponentStyle(eva.style));

It actually works as expected on web! I see atomic css classes and not inline styles. And when I hover on the button, only the css class for the styles that changed will change (such as r-backgroundColor-blabla class). All the other classes for style rules that haven't changed remain the same.

Any time new states are dispatched or a variantGroup prop changes, the styled HOC/decorator should result in component rerender and a new call to this.getComponentStyles(eva.style) so wrapping the object result of that in StyleSheet.create ends up registering the rules as css classes instead of inline styles. For example:

  1. MouseEnter dispatches hover change: https://github.com/akveo/react-native-ui-kitten/blob/1403fdb993f2b6f3fb46fcddfddb185d5652883a/src/components/ui/button/button.component.tsx#L124-L125

  2. styled hoc interaction state is changed so it rerenders component with new eva styles for that state: https://github.com/akveo/react-native-ui-kitten/blob/1403fdb993f2b6f3fb46fcddfddb185d5652883a/src/components/theme/style/styled.tsx#L120-L152

  3. button is rerendered and calls this.getComponentStyle(eva.style) again: https://github.com/akveo/react-native-ui-kitten/blob/1403fdb993f2b6f3fb46fcddfddb185d5652883a/src/components/ui/button/button.component.tsx#L186-L188

I don't know that it's just as simple as wrapping all this.getComponentStyle(eva.style) calls in the components with StyleSheet.create(...) though (if it is, we could just use patch-package and patch ourselves). It appears to work on web, but I'm not sure if there would be performance issues with multiple calls to StyleSheet.create inside rerenders on native. StyleSheet.create is designed to limit trips over the bridge. If it has some internal logic that checks a cache to see if style rules have already been registered, then maybe it would be fine?

RWOverdijk commented 3 years ago

@sschottler Thanks for replying even though it has been a while since you created this issue!

Right now we are instructing it to perform those checks every time you call StyleSheet.create(). So for performance this wouldn't be great. In fact it'd probably be worse than inlining the styles the way we do now.

I'm currently thinking about ways of solving this but I quickly noticed that the way ui-kitten fundamentally works makes this very difficult. I've rewritten this comment around 9 times now (massive walls of text, too) but I keep deleting it because I keep getting stuck coming up with solutions.

The TL;DR; is that I think a fundamental change is needed to get StyleSheet.create() to work for us. Dynamically generating style objects needs to be replaced by dynamically composing style arrays. This approach would no longer be what UI-Kitten currently is.

So instead of forcing it to behave this way right now, I'll instead try to set up a new/different system and offer it up for comparison. Perhaps it explains the ideas I have and we can translate it to a path forward. (Or exposes that I'm overthinking it and I'm just wrong).

Unless you have another good idea that is πŸ˜„

sschottler commented 3 years ago

Hmm...your post helped me think through this a bit more and I have an idea. Please let me know if you've already considered this and ruled it out for constraints I haven't considered yet!

it should be done incrementally (create when not created yet) as it would otherwise be a pretty big hit on initial performance.

  1. What if there was an internal cache for these StyleSheet.create results that all style queries (component+variants+states) go through? It would have similar keys (eg button.filled.danger.disabled) to the internal normalized eva styles object that is built upfront by schemaProcessor.process at either runtime or buildtime in the node_modules/@eva-design/eva/generated.json file.

  2. When the styled HOC/decorator queries for styles here, it will actually check this new cache first. If a query for this particular component+variants+states key has already been made, it will return the result of that StyleSheet.create call, otherwise it will lookup the styles from normalized eva styles object and then call StyleSheet.create and cache result.

  3. Before this new cache can call StyleSheet.create with normalized eva styles for the current component+variants+states, it would need to convert eva mapping tokens to actual style rules (currently done in getComponentStyle methods in components that take mapping tokens like labelMarginBottom: 4 for Input and translate it to label: { marginBottom: 4 } style object). We'd want this translation logic to be colocated with the component and not the styled hoc/decorator since styled is supposed to be generic helper for all components to plugin to eva and would not have knowledge of each specific component's internal structure. So each component would need to make this method available to styled to call in such a way that it doesn't trigger rerender or any performance overhead.

  4. This would only be necessary on web because apparently there isn't a performance benefit to using StyleSheet.create on native anymore (some docs talk about optimizations through bridge, but that appears to have changed):

RWOverdijk commented 3 years ago

Good points! And I thought about the same solution as well, but you'll end up with a lot of duplicate styles. Take a look at the mapping styles ui-kitten generates now. It's a key for every possible appearance/variantGroup combination of which 99% is identical. I understand it's an improvement on what is happening now and I definitely wouldn't stop it, but it's still not an elegant solution.

We could for example have a single styleFactory (a function provided by the component that generates the styles similar to this) which returns styles. We could call it for ever combination (mapping styles) and use the diff between objects to create smaller styles.

This to me sounds like a ton of work to just get rid of duplication in styles. Alternatively we could just accept the way things are now, and move on.

I don't like moving on πŸ˜„

There are some pros and cons to consider here. Perhaps the ability to create new variants by just editing the mapping is not worth the overhead and trouble it brings with it. But that's a discussion about a different topic.

sschottler commented 3 years ago

Yeah, I asked about the repetition here. The approach I proposed above was just an attempt at working with existing structure and not calling StyleSheet.create every rerender, but yes, it would result in repeated checks for the 10 font/border styles or whatever that don't change between hover and default state of button, when really we only care about the few styles that do (backgroundColor, etc.) If UI kitten separated the style diffs between variants and states and default component styles, then more granular StyleSheet.create calls with smaller style objects could be made, but that would require more significant internal restructure. I suspect they build this styles lookup object upfront to avoid runtime overhead of deepmerging in rerenders.

RWOverdijk commented 3 years ago

True. That's what happens when using the exported styles object as well.

I've decided to let go of this for now and instead focus on my own implementation of this. What I've learned thus far is that it's extremely difficult to come up with a nice way of doing this. So I'm leaving this out there for someone else to tackel while I just use a context provider and write my own components.

artyorsh commented 3 years ago

Well what I know is that there is an experimental feature in core RN which might be helpful for moving from the StyleService to StyleSheet. I'm not sure why it's marked experimental and don't really know the technical details of it, but I've seen that a while ago when I had to build a StyleService. Anybody had a chance to use this setStyleAttributePreprocessor?

RWOverdijk commented 3 years ago

I don't think that'll help here, especially since the real issue is with web.

In my opinion the way styles are currently being created would have to be fundamentally changed in order to resolve this issue. Each controlling prop (variantGroup, state, appearance) would need to have its unique style properties (what happens when disabled, when large, when active) added to a StyleSheet.create() object property. Then instead of returning a composed style object UI-Kitten would have to return a composed styles array.

[styles.disabled, styles.primary, styles.checked]

This is a complex task, and one that might not be worth the effort depending on your project's goals. Since I last commented I've switched to a simple react context provider/consumer pair where each stylesheet gets access to the resolved values in the mapping (a factory that gets the theme and the mapping). The returned values are (lazilly) wrapped in a StyleSheet.create() call. That way I can use simple conditionals to add or remove a style while still being able to use variables in my mappings. Sure, I lose the magic of additional styles by just adding a variant or state to my mapping, but it's a lot more lightweight and simple to reason about.

Right now I'm trying to use the eva mapping to create the styles array inside of a react hook. I need to know which prop links to which stylename in my stylesheet factory. It'd be a helper (like useStyled) that should behave the same way ui-kitten does currently. It's challenging though, so if you feel up to the task you can try writing an implementation, too. Then we can compare notes πŸ˜„