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: Implement style-value-parser to stylex normalize-value and improvements in style-value-parser #754

Draft
wants to merge 6 commits into
base: chore/css-value-parser
Choose a base branch
from

Conversation

Prakshal-Jain
Copy link
Contributor

Implementing style-value-parser

  • postcss-value-parser currently reduce the amount of variance that the same CSS value might have. e.g. margin: 0px is converted to margin:0 and extra spaces are stripped out, etc. However, we have to manually define helper functions to cannonicalize CSS values
  • packages/shared/src/utils/normalize-value.js now uses style-value-parser, the package is responsible for parsing CSS values. The goal is to better "cannonicalize" CSS values, and provide a more modular way to scale canonicalization for future.

Behavior

If the value fails to parse, it will default back to use the postcss-value-parser.

Improvements in style-value-parser

  • Created appearance and margins properties

  • Added tests for:

    • appearance.test.js
    • box-shadow.test.js
    • margins.test.js
  • packages/style-value-parser/src/css-types/color.js

    • Fix rgb/rgba CSS color value parsing. This was initially flipped.
    • Fixed the parsing for Hex colors for hex length > 3 characters
    • If alpha=1, use rgb instead of rgba
  • Created CSS Variable (packages/style-value-parser/src/css-types/css-variable.js) css type. It is not being used anywhere yet due to being recursive and not having an end case yet.

  • Improved properties.jsby cleaning up import/exports

@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 16, 2024
Copy link
Contributor

@nmn nmn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Changes look good.
  • We will need some tests to verify that invalid strings fail to parse.
  • All the signals are red. Let's fix that.

@nmn
Copy link
Contributor

nmn commented Oct 18, 2024

There is a concern here: Performance

Adding manual parsing for all style values significantly slows down the time it takes to compile.

Copy link

github-actions bot commented Oct 18, 2024

workflow: benchmarks/size

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

@stylexjs/[email protected] size:compare
./size-compare.js /tmp/tmp.Mvh5tvSsUV /tmp/tmp.xNDuQdzCeM

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 561,465 1.00 -
· minified 10,185,368 10,185,455 1.00 +
rollup-example/.build/stylex.css
· compressed 99,154 97,200 0.98 -
· minified 745,649 740,646 0.99 -

@nmn
Copy link
Contributor

nmn commented Oct 28, 2024

NOTE: The custom style-value-parser causes a significant hit to compile time performance. The rollup example, which compiles a very large bundle went from about 10s to about 3m after using style-value-parser for normalizing values. After adding memoization, I was able to bring down the time to about 1m15s.


This is far too slow to be viable, so I tried lightningcss instead:

The performance impact is negligible with numbers between 10.5 and 12s.


Lightningcss does not have an API to parse style values specifically and any custom logic needs to be implemented as a visitor. The performance is good, but the current implementation is forced to use some hacky string manipulation to extract the style value from the minified CSS generated by LightningCSS. We should keep looking for an even better solution before shipping this.

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