stylex icon indicating copy to clipboard operation
stylex copied to clipboard

[eslint-plugin] Add rule to disallow dynamic styles that could be represented as conditional styles

Open nmn opened this issue 1 year ago • 4 comments

StyleX provides an API to define dynamic styles for the few edge-cases where the value or possible values of a style cannot be known statically when the code is being authored.

This means that values of dynamic styles has to be dynamically computed at runtime and a given set of possible values cannot be known ahead of time.


The Task

There should be an ESLint rule that detects and disallows abuses of dynamic styles when static styles can be applied conditionally instead.

Here's a few possible patterns that should be disallowed:

A dynamic styles where the argument is a boolean

const styles = stylex.create({
  root: (isHovered) => ({
    color: isHovered ? 'grey' : 'black',
  }),
});

This dynamic style can be expressed as conditional styles instead:

const styles = stylex.create({
  root: {
    color: 'black',
  },
  rootHovered: {
    color: 'grey',
  },
});

<div {...stylex.props(styles.root, isHovered && styles.rootHovered)} />

When the dynamic style function is always invoked with a static value

There may be a dynamic style function that is truly dynamic, however, if it is known to always be used locally with static values, then the dynamic style should be replaced with those static styles instead.

nmn avatar Oct 09 '24 03:10 nmn

Could this become an automatic compiler optimization for functional style rules? It would be aligned with where UX/DX perf optimizations are heading with React. The function syntax is quite nice, even for conditional styles that might not be dynamic.

If the function rule is used in the same file where it's defined...

const styles = stylex.create({
  root: (isHighlighted) => ({
    color: isHighlighted ? 'grey' : 'black',
  })
});

function Foo() {
  return <html.div style={styles.root(isHighlighted))} />
}

It would ideally be transformed to the same output as the "conditional" syntax:

const styles = {
  root: {
    $$css: true,
    color: 'xdjwem',
  },
  root_isHighlighted: {
    $$css: true,
    color: 'xpqemf',
  }
});

function Foo() {
  return <html.div style={[ styles.root, isHighlighted && styles.root_isHighlighted  ]} />
}

The function rule could be defined in one file and used in another.

import { menuStyles } from './styles'

function Menu() {
  return <html.div style={menuStyles.root(isExpanded)} />
}

And potentially "memoized" to class names in many cases.

const memoizedStyles = {
  root: {
    $$css: true,
    color: 'xdjwem',
  },
  root_isExpanded: {
    $$css: true,
    color: 'xpqemf',
  }
});
  
export const menuStyles = {
  root: (isExpanded) => isExpanded ? memoizedStyles.root_isExpanded : memoizedStyles.root
});

Using the function rules this way is currently discouraged, but if the compiler could make it more efficient, it could be quite an ergonomic API for libraries using StyleX.

necolas avatar Oct 09 '24 04:10 necolas

Could this become an automatic compiler optimization for functional style rules?

To some extent yes. I'm currently thinking through the patterns that could be detected and optimised.

Some half-formed thoughts:

  • The last example you shared, where the function is preserved with "memoized" objects that are hoisted out is fairly achievable
  • It would work best when all styles within the function are conditional. A mix of conditional and "truly dynamic" styles would be problematic.
  • We would need to detect the number of possible conditions used within the function, as too many conditions within the same function optimised would lead to a combinatorial explosion of code.
    • When optimising stylex.props with statically known styles, we bail out after 4 conditions.

nmn avatar Oct 09 '24 05:10 nmn

Breaking conditional declarations out into individual rules would avoid combinatorial complexity. The function can return an array of styles. It's just like what you'd do by hand if required to manually convert it to the existing "conditional styles" pattern

necolas avatar Oct 09 '24 06:10 necolas

Yeah, that's a great idea and it's something that should be doable without a huge refactor. Let me create a new issue for this.

nmn avatar Oct 09 '24 07:10 nmn