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

fix: BREAKING CHANGE: Themes set *all* variables in a VarGroup. always. #631

Merged
merged 5 commits into from
Nov 1, 2024

Conversation

nmn
Copy link
Contributor

@nmn nmn commented Jul 6, 2024

Partially addresses #601

Terminology:

  • VarGroup - A group of variables created by stylex.defineVars. These variables have default values.
  • Theme - A className generated that sets some or all of the variables in a VarGroup to alternate values.

The Change

Changes the way stylex.createTheme such that when creating a Theme for a given VarGroup, any variables from the VarGroup that are NOT overridden will be set to their default value.

In practice, this means that the variables within a VarGroup will have at most one theme applied.

This also means that generating a theme to reset all variables within a VarGroup is as easy as:

const resetTheme = stylex.createTheme(myVars, {});

stylex.createTheme can be used in any file, so this can be defined in the component file where a theme needs to be reset. Further, defining the same theme in multiple files will not result in duplicate CSS, and so it is safe to repeat this declaration wherever it is needed.


Next Steps

A follow-up PR will detect CSS variables that point to other variables and add a minimal amount of CSS to handle those cases specifically.

@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 Jul 6, 2024
Copy link

github-actions bot commented Jul 6, 2024

compressed-size: runtime library

Size change: 0.00 kB
Total size: 2.52 kB

View unchanged
Filename: gzip (minify) kB size kB change % change
./packages/stylex/lib/stylex.js 1.04 (3.22) 0.00 (0.00) 0.0% (0.0%)
./packages/stylex/lib/StyleXSheet.js 1.48 (3.75) 0.00 (0.00) 0.0% (0.0%)

Copy link

github-actions bot commented Jul 6, 2024

compressed-size: e2e bundles

Size change: 0.00 kB
Total size: 1125.73 kB

View unchanged
Filename: gzip (minify) kB size kB change % change
./apps/rollup-example/.build/bundle.js 1002.49 (10185.35) 0.00 (0.00) 0.0% (0.0%)
./apps/rollup-example/.build/stylex.css 123.24 (774.87) 0.00 (0.00) 0.0% (0.0%)

@nmn nmn requested a review from necolas July 9, 2024 12:01
@necolas
Copy link
Contributor

necolas commented Sep 9, 2024

Having options means that code could be author using one option and then work totally differently if used in a project that sets a different option. Internally we're only going to use one of these so probably best to just add that one. The extra complexity here doesn't seem worth it to me

@nmn
Copy link
Contributor Author

nmn commented Sep 10, 2024

I agree that the complexity isn't worth it. I didn't want to make a decision at the time as I don't agree with how global works as how themes should work. I also verified that other libraries like Vanilla Extract don't do theming the way global does.

Both var or group make sense to me however. var is currently the default and changing the behaviour to group sounds like a good idea to me.

Copy link

github-actions bot commented Oct 28, 2024

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.5wtaVFvR5m /tmp/tmp.N1D7oe8eHG

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

@nmn nmn changed the title feat: add theme override behaviour configuration fix: BREAKING CHANGE: Themes set *all* variables in a VarGroup. always. Oct 31, 2024
@nmn nmn merged commit 5f14ff5 into main Nov 1, 2024
8 checks passed
@nmn nmn deleted the feat/theme-exclusivity branch November 1, 2024 19:20
Samantha-Zhan pushed a commit that referenced this pull request Nov 6, 2024
)

* feat: add theme override behaviour configuration
* feat: missing vars in themes are set to their default values
* fix: resolve the correct __themeName__ for VarGroups
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