facebook / stylex

StyleX is the styling system for ambitious user interfaces.
https://stylexjs.com
MIT License
8.2k stars 304 forks source link

Support of swapping underlying variables while using hierarchical variable references #601

Open zetavg opened 2 weeks ago

zetavg commented 2 weeks ago

Describe the feature request

Update: as @aspizu mentioned, see #566 for a more concise description of the same situation.

Background

In some design systems such as Material Design, SAP Fiori and many more, there’s this concept called Design Token Hierarchy, where high level and more semantic tokens can reference low-level tokens.

(Image from https://stefaniefluin.medium.com/the-pyramid-design-token-structure-the-best-way-to-format-organize-and-name-your-design-tokens-ca81b9d8836d)

To my knowledge, we can define hierarchical tokens in StyleX like this - for system and reference tokens:

// tokens.stylex.ts

import * as stylex from '@stylexjs/stylex';

/** Reference tokens for color palette */
export const colorPalette = stylex.defineVars({
  blue: '#007AFF',
  indigo: '#5856D6',
  white: '#F2F2F7',
  // ...
});

/** System tokens for color */
export const systemColors = stylex.defineVars({
  primary: colorPalette.blue,
  onPrimary: colorPalette.white,
  // ...
});

And for component tokens:

// components/Button/tokens.stylex.ts

import * as stylex from '@stylexjs/stylex';

import { systemColors } from '../../tokens.stylex';

export const buttonTokens = stylex.defineVars({
  primaryColor: systemColors.primary,
  primaryLabelColor: systemColors.onPrimary,
  // ...
});

Which, for example, constructs a token hierarchy as:

(Component Token)              (System Token)            (Reference Token)

buttonTokens.primaryColor <─── systemColors.primary <─── colorPalette.blue <─── '#007AFF'

The Problem

Using themes that override tokens (variables) that are referenced by other tokens (variables) might not work as expected. For example, if we want to apply a different color theme:

import * as stylex from '@stylexjs/stylex';

import Button from './components/Button';
import { colorPalette, systemColors } from './tokens.stylex';

const indigoTheme = stylex.createTheme(systemColors, {
  primary: colorPalette.indigo,
});

function App() {
  return (
    <>
      <div {...stylex.props(indigoTheme)}>
        <h2>Button with Indigo Theme</h2>
        <p>
          In this div, a theme is applied which overrides <code>systemColors.primary</code> to <code>colorPalette.indigo</code>.
        </p>
        <Button label="Indigo button" />
      </div>
    </>
  );
}

We may expect an indigo button:

(Component Token)              (System Token)            (Reference Token)

buttonTokens.primaryColor <─── systemColors.primary <─╮x colorPalette.blue <─── '#007AFF'
                                                      │
                                                      ╰─ colorPalette.indigo <─ '#5856D6'

But it’s not the case. The button will still be blue, due to how CSS variables work:

:root {
    --systemColors_primary: var(--colorPalette_blue);
    /* ... */          │
}                      ╰───────────────────────────────╮
                                                       │
:root {                                                ↓
    --buttonTokens_primaryColor: var(--systemColors_primary);
    /* ... */               │
}                           ╰─────────────────────────────╮
                                                          │
.indigoTheme {                                            │
    --systemColors_primary: var(--colorPalette_indigo);   │
    /* ... */                                             │
}                                      ╭──────────────────╯
                                       │
.button {                              ↓
    color: var(--buttonTokens_primaryColor);
    /* ... */
}

(In reality, StyleX will generate unique IDs for class names and variable names; here, we write them as recognizable names just for readability.)

To make this work, we may need to modify StyleX to re-declare all the variables that reference overwritten variables when declaring themes, so that:

:root {
    --systemColors_primary: var(--colorPalette_blue);
    /* ... */
}

:root {
    --buttonTokens_primaryColor: var(--systemColors_primary);
    /* ... */
}

.indigoTheme {
    --systemColors_primary: var(--colorPalette_indigo);
                       ╰──────────────────────╮
    /* Re-declare */                          ↓
    --buttonTokens_primaryColor: var(--systemColors_primary);
    /* ... */                │
}                            ╰─────────╮
                                       │
.button {                              ↓
    color: var(--buttonTokens_primaryColor);
    /* ... */
}

Repro

I made a repro here: https://github.com/zetavg/stylex-token-hierarchy-esbuild-example. It’s based on the esbuild-example.


I’m not sure if this is a thing that can be improved, is there already a solution, or it’s designed this way on purpose.

Instead of re-declaring CSS variables, there’s another solution in my mind which is done by adding class names, but I’m unsure about the performance and scalability concerns with either way.

necolas commented 2 weeks ago

This is probably a bug given how you'd expect things to work.

I think we can get the expected result by including the base theme's class alongside the sub-theme class. StyleX could do this automatically when returning createTheme styles.

https://codesandbox.io/p/sandbox/stylex-vars-65yyjg?file=%2Findex.html%3A10%2C9

There has been previous talk about needing to expose the base theme as a style anyway.

aspizu commented 2 weeks ago

The problem comes from CSS itself (#566). There might be issues with re-declaring custom properties which inherit other custom properties, What if there is a theme which changes that particular custom property to have a constant fixed value? The compiler would have to figure out how to handle dynamic values set by javascript code.

necolas commented 2 weeks ago

@aspizu your example and expected result is also possible: https://codesandbox.io/p/sandbox/stylex-vars-65yyjg?file=%2Findex.html%3A5%2C13. What are the cases where this approach wouldn't work?

zetavg commented 2 weeks ago

Thanks @necolas, and @aspizu for pointing out what I've missed.

I wrote a quick patch to StyleX and can now make it achieve what I expected (output of the patched version of StyleX: https://codesandbox.io/p/sandbox/stylex-token-hierarchy-esbuild-example-output-240601-vdwvtg).

Maybe such behavior can be made a config to enable? Or, due to the principle to avoid global configuration for StyleX, let stylex.createTheme accept an option argument for it to also add the base theme class name?

necolas commented 2 weeks ago

Nice. I think this is how StyleX theming should work by default, but let's see what Naman thinks

olivierpascal commented 2 weeks ago

I like this!

nmn commented 2 weeks ago

I've given this thought before and it's not as simple. The idea here is that variables should work like reactive values. This sometimes feels like the way things should work but this is not always the case.

I think safest way to make this work is by using defineConsts instead of defineVars for certain layers. This would obviously mean that the constant cannot be redefined. But for constants we could basically erase them at compile time so they are "reactive".

Would that be a good solution?

necolas commented 2 weeks ago

What's wrong with the approach we discussed above? And what is the output code that makes values "reactive"?

nmn commented 2 weeks ago

And what is the output code that makes values "reactive"?

My proposal is that we can do handle constants differently where they don't exist after compilation at all.

So taking the original example:

// tokens.stylex.ts

import * as stylex from '@stylexjs/stylex';

/** Reference tokens for color palette */
export const colorPalette = stylex.defineVars({
  blue: '#007AFF',
  indigo: '#5856D6',
  white: '#F2F2F7',
  // ...
});

/** System tokens for color */
export const systemColors = stylex.defineVars({
  primary: colorPalette.blue,
  onPrimary: colorPalette.white,
  // ...
});

export const buttonTokens = stylex.defineConsts({
  primaryColor: systemColors.primary,
  primaryLabelColor: systemColors.onPrimary,
  // ...
});

This will generate following CSS:

:root {
    --systemColors_primary: var(--colorPalette_blue);
    /* ... */          │
}                      ╰───────────────────────────────╮
/*                                                     │
:root {                                                ↓
    --buttonTokens_primaryColor: var(--systemColors_primary);
    ... */                  │
}                           ╰─────────────────────────────╮
*/                                                        │
.indigoTheme {                                            │
    --systemColors_primary: var(--colorPalette_indigo);   │
    /* ... */                                             │
}                                      ╭──────────────────╯
                                       │
.button {                              ↓
    color: var(--systemColors_primary); /* var(--buttonTokens_primaryColor) */
    /* ... */
}

So essentially, any "constant" is replaced by its value at compile time so the extra layers are a pure abstraction for DX only and does not exist at all at runtime.

IMO, this should solve the core problem described here. In fact, systemColors could probably be defined as constants as well for better efficiency.

I'm thinking of some examples to explain why making the system work as the OP suggested is problematic.

What's wrong with the approach we discussed above?

This codesandbox might show the problem with your approach.

There is an edge-case with applying any other theme. As applying the x className in your example wipes out all other themes and resets everything back to default.