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

feat: support reactive themes up to 4 levels deep #761

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

nmn
Copy link
Contributor

@nmn nmn commented Oct 31, 2024

fixes #601

This PR finally implements reactive themes where variables work the way you expect. However, as a practical limitation, it only supports 4 layers of nested themes for a VarGroup that uses values from a separate VarGroup to define themes.

This can be a bit hard to explain, so here's an example.

Say there are two VarGroups, globalTokens and pageTokens:

export const globalTokens = stylex.defineVars({
  primary: 'black',
  wash: 'lightgrey',
});

export const pageTokens = stylex.defineVars({
  fg: 'black',
  bg: 'white',
});

And there are themes for these two VarGroups as well:

export const redGlobals = stylex.createTheme(globalTokens, {
  primary: 'red',
  wash: 'lightpink',
});

export const pageTheme1 = stylex.createTheme(pageTokens, {
  fg: globalTokens.primary,
  bg: globalTokens.wash,
});

Now consider the following usage examples:

<div {...styles.props(redGlobals)}>
  <div {...styles.props(pageTheme1)}>
    `fg` should be "red"
    `bg` should be "lightpink"
  </div>
</div>

In this first example because the theme for globalTokens is applied higher up the tree than the pageTokens, everything works as expected. When pageTheme1 is applied, it resolves to the values from redGlobals as they are already available.


Now, here is the example where we can see the updated behaviour:

<div {...styles.props(pageTheme1)}>
  <div {...styles.props(redGlobals)}>
    `fg` should be "red". It was "black" before this PR.
    `bg` should be "lightpink". It was "lightgreay" before this PR.
  </div>
</div>

Here, the pageTheme is applied higher up the tree. The way that CSS works, this would mean that it set the pageTokens to values from the default values of globalTokens, which is "black" and "lightgrey".

However, when redGlobals is applied further down the tree, most devs expect the variables to behave reactively and expect the "pageTokens" that depend on "globalTokens" to resolve to the nearest value.

After this PR, they do.


How does this work?

Relevant Background

Every .defineVars call creates a unique className for the set of variables. Let's assume the classNames are ".globalTokens" and ".pageTokens" respectively. (The real classNames are hashes).

Every .createTheme call uses this className alongside the className created for the generated theme. Let's assume that:

  • redGlobals sets the classNames "redGlobals globalTokens"
  • pageTheme1 sets the classNames "pageTheme1 pageTokens"

NOTE: the themes return TWO classNames each. This is how any variables that are omitted in the theme are automatically set to their default values from the defineVars call.

Dealing with "dependencies"

This PR adds code to track the "dependencies" of a Theme. In the example above, pageTheme1 depends on values from globalTokens.

So to ensure that once pageTheme1 is applied it always uses the most "current" value for globalTokens, CSS descendent selectors are used:

.pageTheme1,
.pageTheme1 .globalTokens {
  --fg: var(--globalTokens_primary);
  --bg: var(--globalTokens_wash);
}

Since any Theme for globalTokens will also apply the .globalTokens className, we can automatically re-apply pageTheme1 whenever globalTokens is themed further down the tree.

Accounting for nested themes

So far, we only considered the case where there is only a single theme for pageTokens being applied. However, it is possible that there are many themes for the same VarGroup that are applied on nested elements:

<div {...styles.props(pageTheme3)}>
  <div {...styles.props(pageTheme2)}>
    <div {...styles.props(pageTheme1)}>
      <div {...styles.props(redGlobals)}>
        `fg` should be "red". It was "black" before this PR.
        `bg` should be "lightpink". It was "lightgreay" before this PR.
      </div>
    </div>
  </div>
</div>

With the CSS selector above, we would end up applying the styles for all three pageToken themes. What we want is the inner-most theme for pageTokens to take precedence. Currently, the only way in CSS to create a so-called "nearest parent" selector is by using the new feature @scope. @scope is a very new feature that is not even supported in Firefox yet. Being an at-rule, it also necessitates repeating the entire CSS again.

As a practical workaround, this PR implements the feature for upto 4 nested themes, and implements them with bigger selectors that will win via specificity:

.pageTheme1,
.pageTheme1 .globalTokens, 
.pageTokens .pageTheme1 .globalTokens,
.pageTokens .pageTokens .pageTheme1 .globalTokens,
.pageTokens .pageTokens .pageTokens .pageTheme1 .globalTokens {
  --fg: var(--globalTokens_primary);
  --bg: var(--globalTokens_wash);
}

Since every theme for pageTokens will also set the className pageTokens, every time you nest a theme for the same VarGroup, a selector with a higher specificity will win.

This means that as long as you don't nest themes for the same VarGroup more than four times, things will work as expected.


No unnecessary complexity

The compiler detects when CSS variables from another VarGroup are used when defining the values of a theme. It only adds this complexity to the CSS when it is needed. If you don't define variables that point to other variables, no additional complex selectors will be created.

What's next?

This PR has implemented the feature for createTheme, but the same feature is also needed for defineVars itself. We need to account for the default values of a VarGroup itself depending on variables from another VarGroup.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 31, 2024
Copy link

workflow: benchmarks/size

Comparison of minified (terser) and compressed (brotli) size results, measured in bytes. Smaller is better.

@stylexjs/scripts@0.9.0-beta.1 size:compare
./size-compare.js /tmp/tmp.ba0J6kR9FB /tmp/tmp.zGmNt9D2JA

Results Base Patch Ratio
stylex/lib/stylex.js
· compressed 729 729 1.00
· minified 2,541 2,541 1.00
stylex/lib/StyleXSheet.js
· compressed 1,266 1,266 1.00
· minified 3,776 3,776 1.00
rollup-example/.build/bundle.js
· compressed 563,025 563,025 1.00
· minified 10,185,368 10,185,368 1.00
rollup-example/.build/stylex.css
· compressed 99,154 99,154 1.00
· minified 745,649 745,649 1.00

@necolas
Copy link
Contributor

necolas commented Oct 31, 2024

I think this is back to creating a situation where themes get blended. That is a complexity that is best avoided IMO.

The example provided is essentially 2 theme partials being nested. In practice people will create one big theme that's an array of theme partials. If the API worked as you described, then part of that theme could be overridden within the subtree, creating a blend of themes rather than mutually exclusive themes

@nmn
Copy link
Contributor Author

nmn commented Oct 31, 2024

@necolas I think I failed to explain the work correctly in the description above. There should NO blending of themes. There should only ever be exactly ONE theme whose values are active.

The work here is only about dealing with the situation discussed in #601 where variables A can point to other variables B, but if variables B are themed, then, by default, CSS does not update the values of variables A.

I still have to make the changes to defineVars which should ensure that themes stay exclusive and don't get blended.


Now, there are some edge-cases I'm thinking through and it's possible this solution won't work out, but we're on the same page about how the themes should work.


Update:

You're right. This approach won't work. However, I do have another path that I think should work.

Base automatically changed from feat/theme-exclusivity to main November 1, 2024 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants