Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ESLint Rule to disallow dynamic styles that could be represented as conditional styles #727

Open
nmn opened this issue Oct 9, 2024 · 4 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@nmn
Copy link
Contributor

nmn commented Oct 9, 2024

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 nmn changed the title Disallow conditions that set static values within dynamic styles ESLint Rule to disallow dynamic styles that could be represented as conditional styles Oct 9, 2024
@nmn nmn added enhancement New feature or request help wanted Extra attention is needed labels Oct 9, 2024
@necolas
Copy link
Contributor

necolas commented Oct 9, 2024

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.

@nmn
Copy link
Contributor Author

nmn commented Oct 9, 2024

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.

@necolas
Copy link
Contributor

necolas commented Oct 9, 2024

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

@nmn
Copy link
Contributor Author

nmn commented Oct 9, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants