nandorojo / dripsy

🍷 Responsive, unstyled UI primitives for React Native + Web.
https://dripsy.xyz
MIT License
2.01k stars 80 forks source link

Question: inline style generation #88

Closed macieklad closed 3 years ago

macieklad commented 3 years ago

On the web, each element that has the sx prop generates whole copy of its styles as inline ones, is that intended or should it be converted into styled components classnames? I wanted to ask before diving deeper into the potential performance issues it may create.

Example: devtools

nandorojo commented 3 years ago

This is not intended, I've noticed it too. Perhaps it's due to not using StyleSheet.create under the hood. I've considered adding that before. Let me know if you find that it negatively affects performance/if you know a solution.

macieklad commented 3 years ago

Great! I will keep it in mind and will report back as my project grows.

fortezhuo commented 3 years ago

@nandorojo I have found some article about inline styles..

https://developers.google.com/speed/docs/insights/OptimizeCSSDelivery#CSSattributes

Don't inline CSS attributes
Inlining CSS attributes on HTML elements (e.g., <p style=...>) should be avoided where possible, as this often leads to unnecessary code duplication. Further, inline CSS on HTML elements is blocked by default with Content Security Policy (CSP). 

https://www.sderosiaux.com/articles/2015/08/17/react-inline-styles-vs-css-stupid-benchmark/

Inline styles take way more size in the DOM, are converted more slowly from VDOM (have probably a bigger impact on memory), and take more time to be handled by the browser.
nandorojo commented 3 years ago

Is StyleSheet.create safe to use in render?

fortezhuo commented 3 years ago

I don't think StyleSheet.create is safe to use in render.. But if we don't have any choice and must put StyleSheet.create inside render, we can wrap it with useMemo

nandorojo commented 3 years ago

We'll probably have to stringify the result and create the stylesheet in useMemo.

nandorojo commented 3 years ago

The best would really be to have a Babel plugin/macro that checks any static sx props and turns them into StyleSheet.create. I've discussed this with @natew, who made a cool and performant Babel solution for SnackUI.

Stuff like this might be what we need: https://youtu.be/9xTZSW2soBM

nandorojo commented 3 years ago

https://www.npmjs.com/package/babel-plugin-rn-extract-style

Are there any babel experts out there?

nandorojo commented 3 years ago

FWIW, this is only a concern on web. Stylesheet.create does nothing on native.

nandorojo commented 3 years ago

Could you confirm that, sometimes the sx prop currently does create classNames, and other times it does not?

fortezhuo commented 3 years ago

FWIW, this is only a concern on web. Stylesheet.create does nothing on native.

I have found interested answer from https://stackoverflow.com/questions/39336266/react-native-inline-styles-and-performance

As mentioned in the comments of the selected answer, the performance comment has been removed from the RN docs, but in my project (a mobile game using RN that uses multiple active Animated components), using Stylesheet instead of inline styles helps performance.

My main view where around 5-10 Animated components are active, the UI FPS drops to 15-18 when I use inline styles, but if I use Stylesheet it has a 60fps with little to no stagger. Note that I had to implement this in most components especially the Animated components and its child components in order to see the improvements, I still have some inline styles lying around in other components.

When it comes to performance, your UI FPS is affected by your js activity. So reducing/optimizing JS load will help to keep your FPS perform better. In my project, some style values are computed, but they only need to be computed during the initial load. When I use inline styles, these values are computed every re-render, so using stylesheets makes more sense since there will be no recomputing. This is especially important when handling image assets.

Bottom line, use Stylesheet when you can. It will help in the long run and avoid any unnecessary updates from inline to stylesheet.

Could you confirm that, sometimes the sx prop currently does create classNames, and other times it does not? Yes. I can confirm that. I have 2 dripsy component nested. And TextInput has className on web

// RNBase is myown skeleton component with linear gradient that wrapped as dripsy component
const Base = createThemedComponent(RNBase, {
  themeKey: "input",
})

const TextInput = createThemedComponent(RNTextInput, {
  themeKey: "input",
  defaultVariant: "text",
})
<Base
  isLoading={isLoading}
  variant={disabled || isUpdating ? "disabled" : "default"}
  sx={sxContainer}
>
  <TextInput
    ref={ref}
    placeholderTextColor={theme.colors.gray[7]}
    sx={sx}
    editable={!disabled && !isUpdating && editable}
    {...{ ...props, onFocus }}
  />
</Base>

image

nandorojo commented 3 years ago

Stylesheet doesn't do anything special. You can look at the source code. It's just an object. Maybe it's better to use fewer object references. But you can achieve that by just creating your sx props outside of render, I assume.

I’ll look into memoizing based on a stringified style when I have time.

nandorojo commented 3 years ago

I'm going to release a memoized style to canary soon.

Another thing to look into is creating a babel plugin that statically analyzes every use of the sx prop, and turns it into a StyleSheet.create at build time.

This would require a breaking change, where the style prop is no longer themed. However, this is undocumented, so I doubt anyone is using it.

One example is this library: https://github.com/teod/babel-plugin-rn-extract-style#readme

Basically, if someone forked that library for Dripsy, it would do the exact same thing, with some changes:

  1. Rather than check the style prop, it checks sx
  2. Rather than just take the prop and put it in StyleSheet.create, it would first need to apply the css function to it.
  3. Pass the result to style prop

The css function requires a theme, so we'd probably need to pass your app's theme to the babel plugin in babel.config.js. Something like:

module.exports = {
  plugins: [['dripsy/babel', { theme: require('./src/theme').default }]]
}

There was talk about this with @cmaycumber on another PR. I don't have any Babel experience, though, so I don't think this is realistic for me to be able to build, at least not any time soon. So hopefully the memoization solution I'll release soon will fix our current inline style issues.

nandorojo commented 3 years ago

Hey everyone,

I just released a new version of dripsy.

Please try it out with yarn add dripsy@canary. It implements an aggressive cache that deep-equal checks the resulting style. I believe it's gotten rid of my inline styles, but I'd like confirmation from you so I know it's good to merge to latest.

If you're upgrading from v1.4 or lower, and you're using Expo Web, please follow these new web instructions: https://github.com/nandorojo/dripsy#for-expo-web--react-native-web-non-ssr-apps

nandorojo commented 3 years ago

As one example, the previous push turned this:

<input
  placeholder="Search artists to book..."
  autocapitalize="sentences"
  autocomplete="on"
  autocorrect="on"
  dir="auto"
  spellcheck="true"
  type="text"
  class="css-11aywtz r-1es4dsl r-q4m81j"
  value=""
  style="color: rgb(255, 255, 255); font-family: circ; font-size: 18px; padding: 12px 8px; transition-duration: 0.8s; transition-property: border-color, background-color, color; transition-timing-function: cubic-bezier(0.2, 0.8, 0.2, 1);"
/>

Into this:

<input
  placeholder="Search artists to book..."
  autocapitalize="sentences"
  autocomplete="on"
  autocorrect="on"
  dir="auto"
  spellcheck="true"
  type="text"
  class="css-textinput-11aywtz r-color-jwli3a r-fontFamily-1pq47yg r-fontSize-1i10wst r-paddingLeft-1m04atk r-paddingRight-1pyaxff r-placeholderTextColor-1es4dsl r-textAlign-q4m81j r-transitionDuration-jkls63 r-transitionProperty-zt0yw4 r-transitionTimingFunction-1g21wze"
  value=""
  style="padding-bottom: 12px; padding-top: 12px;"
/>

I'm not exactly sure why that's happening with padding; perhaps it's due to some sort of aliasing of using px. I'll investigate that too.

nandorojo commented 3 years ago

Ah, the inline style happened from using a responsive style array. I'll have to figure that out later, it's pretty complex. But for non-responsive styles, they should all now be cached and turned into class names.

nandorojo commented 3 years ago

Before

https://user-images.githubusercontent.com/13172299/115423417-cde98b80-a1cb-11eb-9ac5-027f67d3e726.mp4

After

https://user-images.githubusercontent.com/13172299/115423449-d4780300-a1cb-11eb-839d-d2c76ce7d38a.mp4

🥳

nandorojo commented 3 years ago

Published in dripsy@1.5.8

nandorojo commented 3 years ago

1.5.10 adds className support for responsive styles too. Every sx prop now has cached class names.