Skip to content

feat: create media query transformer to ensure last query wins #898

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

Merged
merged 1 commit into from
Apr 11, 2025

Conversation

mellyeliu
Copy link
Member

@mellyeliu mellyeliu commented Feb 22, 2025

create media query transformer to ensure “last query wins” logic

Context

Now that we have a usable media query parser in #743, we can finally implement last media query wins logic.

Implementation

High level approach: traverse through styles, search for media queries, do two passes at each node - one to build up a list of negations for each query, and another to apply the negation.

  1. traverse nested styles with dfs

    • dfs to find all nested media queries
    • extract and flatten all media queries per into list for easier transformation
  2. iterate media queries in reverse order

    • loop through the extracted media queries from last to first
  3. accumulate negations
    -backwards, for single pass

  • build up a list of negations from previous queries during reverse iteration
    • apply these negations to prevent earlier media queries from overriding more specific ones defined later
  1. construct updated media queries
    • forwards order, because we need to reassign the object keys in the same order we see them to avoid breaking js object keys order
    • for each media query: use MediaQuery.parser.parseToEnd to convert query string into object representation
  • append all accumulated negations to queries
    • stringify media query usingMediaQuery.toString()
    • append current queries to negations
  1. return updated styles object with negated media queries

Testing

Use json.stringify to ensure object key order is maintained

Scenarios like:

  • basic usage: multiple widths
  • basic usage: complex styles object
  • basic usage: lots and lots of widths
  • nested queries
  • handles comma-separated (or) media queries
  • handles and media queries
  • keywords
  • combination of keywords and > rules

Missing:

  • Nested not cases blocked on parsing issues

Pre-flight checklist

@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 Feb 22, 2025
@mellyeliu mellyeliu changed the base branch from main to chore/css-value-parser February 22, 2025 12:24
@mellyeliu mellyeliu changed the title Media transform feat: refactor media query transformer to ensure last query wins Feb 22, 2025
@mellyeliu mellyeliu changed the title feat: refactor media query transformer to ensure last query wins feat: create media query transformer to ensure last query wins Feb 22, 2025
Copy link

github-actions bot commented Feb 22, 2025

workflow: benchmarks/size

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

[email protected] compare
node ./compare.js /tmp/tmp.YE2LaIJ6Uz /tmp/tmp.cZHeGXE0mI

Results Base Patch Ratio
stylex/lib/stylex.js
· compressed 985 985 1.00
· minified 3,154 3,154 1.00
stylex/lib/StyleXSheet.js
· compressed 1,266 1,266 1.00
· minified 3,776 3,776 1.00
benchmarks/size/.build/bundle.js
· compressed 537,611 537,611 1.00
· minified 7,435,904 7,435,904 1.00
benchmarks/size/.build/stylex.css
· compressed 100,609 100,609 1.00
· minified 755,721 755,721 1.00

@mellyeliu mellyeliu changed the title feat: create media query transformer to ensure last query wins draft feat: create media query transformer to ensure last query wins Feb 22, 2025
@mellyeliu mellyeliu changed the title draft feat: create media query transformer to ensure last query wins feat: create media query transformer to ensure last query wins Feb 25, 2025
@mellyeliu mellyeliu marked this pull request as ready for review February 25, 2025 08:57
Copy link
Contributor

@necolas necolas left a comment

Choose a reason for hiding this comment

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

What does "last wins" mean exactly in this context? FWIW, the "last wins" approach to property merging in StyleX is only true for properties with the same name. The way we handle short/long form merging (inc within a single style object) is really problematic and something we're trying to undo. So if last-wins for MQ is a local concern within a style, I'm concerned that will also be problematic. Anything that relies on order of keys in authored code is going to have those problems

const expectedStyles = {
gridColumn: {
default: '1 / 2',
'@media (max-width: 1440px) and (not (max-width: 1024px)) and (not (max-width: 768px))':
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be concerned at how bloated generated code gets with this approach

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a bit of a work in progress where we will be simplifying the media queries as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

the doc here provides a bit more context - https://docs.google.com/document/d/1KlMnWz__zVBNcH_HQmd64sPEFf6AdaXeG46rL_eU5ec/edit?tab=t.uwqdsvafq1l7#heading=h.m5zrpmurwti

the plan is to eventually rewrite the media query parser to do the simplification step for us, or to pass this return to another fn to do a second round of parsing

@nmn nmn force-pushed the chore/css-value-parser branch 3 times, most recently from 56c7fe9 to 2187f31 Compare March 4, 2025 23:47
Base automatically changed from chore/css-value-parser to main March 11, 2025 08:02
@mellyeliu
Copy link
Member Author

updated implementation to ensure object key ordering is preserved

@mellyeliu mellyeliu force-pushed the media-query-parser-simplify branch from 0d5a240 to 5a6beb8 Compare March 28, 2025 21:29
@mellyeliu mellyeliu force-pushed the media-query-parser-simplify branch 3 times, most recently from c230391 to 3e2250d Compare March 28, 2025 21:39
Base automatically changed from media-query-parser-simplify to main March 28, 2025 22:24
Copy link

github-actions bot commented Apr 10, 2025

workflow: benchmarks/perf

Comparison of performance test results, measured in operations per second. Larger is better.

[email protected] compare
node ./compare.js /tmp/tmp.mOaO0JYlzi /tmp/tmp.c7c8WZQj4U

Results Base Patch Ratio
babel-plugin: stylex.create
· basic create 537 540 1.01 +
· complex create 198 197 0.99 -
babel-plugin: stylex.createTheme
· basic themes 464 462 1.00 -
· complex themes 43 41 0.95 -

@mellyeliu
Copy link
Member Author

mellyeliu commented Apr 10, 2025

ignore legacy media query syntax by checking dfs depth (>2)
ping @nmn pls review 👻

Copy link
Collaborator

@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.

Please remove the console.logs at least before merging.

Further code improvements can be made later.

@mellyeliu mellyeliu merged commit 5e0bbe9 into main Apr 11, 2025
9 checks passed
@mellyeliu mellyeliu deleted the media-transform branch April 11, 2025 04:01
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.

4 participants