feat: add support for variables in media queries#274
Conversation
- Rename css-variables-media-queries test files to variables - Maintains same test coverage with clearer naming
📝 WalkthroughWalkthroughRefactors media-query handling to recursively evaluate nested MediaCondition nodes, tracks declarations tied to media queries for deferred materialization, narrows MediaQueryResolver width types to number|null, and adds comprehensive media-query tests (min/max-width, orientation, color-scheme, combined) to validate dynamic updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Style Request
participant Processor as ProcessorBuilder
participant MQ as MediaQueryProcessor
participant Store as DeclarationStore
Client->>Processor: addDeclaration(prop, value, mq?)
alt declaration has media-query
Processor->>Processor: parseDeclaration()
Processor->>Store: enqueue in varsWithMediaQueries[class][prop]
Processor->>Processor: record pendingVarReferences[class] if value references var
else no media-query
Processor->>Processor: style[property] = value
end
Processor->>MQ: processMQ(mediaQueryNode)
MQ->>MQ: for-of conditions -> processCondition() (recursive)
note right of MQ `#ffd9b3`: evaluate features\n(min/max-width, orientation, color-scheme)
MQ-->>Processor: return accumulated mq metadata
Processor->>Processor: materialize varsWithMediaQueries for each class
Processor->>Store: merge materialized var styles into class styles
Processor-->>Client: resolved styles (including media-query-derived variables)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
packages/uniwind/src/metro/processor/mq.ts (1)
72-86: Verify default width values match type expectations.The initial values use
minWidth: 0andmaxWidth: Number.MAX_VALUE, but theMediaQueryResolvertype defines these asnumber | null. While this works functionally (0 and MAX_VALUE act as "no constraint"), usingnullmight be more semantically accurate for "no constraint specified."packages/uniwind/tests/media-queries/orientation.test.tsx (3)
26-30: Consider avoidingeval()for test initialization.While this is test code with controlled input,
eval()is flagged by static analysis tools and can be avoided. Consider exposing a method onUniwindthat accepts the parsed object directly, or usingnew Function()which is slightly safer for this pattern:🔎 Alternative approach
- eval( - `const { Uniwind } = require('../../src/core/config/config.native'); - Uniwind.__reinit(rt => ${virtualCode}, ['light', 'dark']); - `, - ) + const { Uniwind } = require('../../src/core/config/config.native') + const initFn = new Function('rt', `return ${virtualCode}`) + Uniwind.__reinit(initFn, ['light', 'dark'])This same pattern appears in other test files in this PR. If refactored, consider extracting to a shared test utility.
63-85: Good dynamic update test, but consider using rerender instead of re-mounting.The test re-renders by calling
renderUniwindagain after orientation change, which creates a new component instance. If testing reactive updates on the same component, consider usingrerenderfrom the render result:const { getStylesFromId, rerender } = renderUniwind(...) // ... change orientation ... rerender(<View className="text-base" testID="text-base" />)This would better test the reactive behavior. However, if the current approach matches the actual user behavior pattern, the test is still valid.
103-113: Clarify test expectation: orientation vs min-width precedence.The test name says "orientation takes precedence over min-width when both match separately," but width 500 may not match the min-width breakpoint in the CSS. The expected result (40) suggests orientation is being applied, but without seeing the CSS file, it's unclear if this tests precedence or simply that orientation applies when min-width doesn't match.
Consider adding a comment clarifying the CSS conditions or adjusting the test name.
packages/uniwind/tests/media-queries/max-width.test.tsx (1)
26-30: Sameeval()pattern as other test files.See the recommendation in
orientation.test.tsxfor avoidingeval(). Consider extracting a shared test utility if refactoring.packages/uniwind/tests/media-queries/combined.test.tsx (2)
1-10: Missingactimport if dynamic updates are added later.Unlike other test files in this PR, this file doesn't import
actfrom@testing-library/react-native. Currently, none of the tests require it since they don't test dynamic updates. If such tests are added later, the import will be needed.
25-29: Sameeval()pattern as other test files.See the recommendation in
orientation.test.tsxfor avoidingeval(). Consider extracting a shared test utility.packages/uniwind/tests/media-queries/min-width.test.tsx (1)
26-30: Sameeval()pattern as other test files.See the recommendation in
orientation.test.tsxfor avoidingeval(). This is the fourth instance — strongly consider extracting to a shared utility.packages/uniwind/src/metro/processor/processor.ts (2)
59-61: Consider addingplatformto media query detection.
hasMediaQuerychecksminWidth,maxWidth,orientation, andcolorScheme, butplatformis also a media-query-like constraint. If platform-specific variables should be included in this flow, consider addingmq.platform !== nullto the check.
114-133: Regex pattern for variable reference detection looks correct.The regex
this\[(.*?)\]correctly matches the template literal accessor pattern used for variable references. The non-greedy*?ensures it captures just the variable name.One edge case: if
valuecontains multiple variable references (e.g.,calc(this[\--a`] + this[`--b`])), only the first will be captured. If multiple references per value are possible, consider usingmatchAll`:const matches = typeof value === 'string' ? [...value.matchAll(/this\[`(.*?)`\]/g)] : [] for (const match of matches) { // process match[1] }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/uniwind/src/core/native/store.tspackages/uniwind/src/metro/processor/mq.tspackages/uniwind/src/metro/processor/processor.tspackages/uniwind/src/metro/types.tspackages/uniwind/tests/media-queries/color-scheme.test.csspackages/uniwind/tests/media-queries/color-scheme.test.tsxpackages/uniwind/tests/media-queries/combined.test.csspackages/uniwind/tests/media-queries/combined.test.tsxpackages/uniwind/tests/media-queries/max-width.test.csspackages/uniwind/tests/media-queries/max-width.test.tsxpackages/uniwind/tests/media-queries/min-width.test.csspackages/uniwind/tests/media-queries/min-width.test.tsxpackages/uniwind/tests/media-queries/orientation.test.csspackages/uniwind/tests/media-queries/orientation.test.tsx
🧰 Additional context used
🧬 Code graph analysis (7)
packages/uniwind/tests/media-queries/orientation.test.tsx (5)
packages/uniwind/src/metro/compileVirtual.ts (1)
compileVirtual(21-74)packages/uniwind/src/components/index.ts (1)
Platform(193-195)packages/uniwind/src/core/native/store.ts (1)
UniwindStore(222-222)packages/uniwind/src/core/listener.ts (1)
UniwindListener(59-59)packages/uniwind/tests/utils.ts (1)
renderUniwind(6-18)
packages/uniwind/tests/media-queries/combined.test.css (3)
packages/uniwind/src/css/themes.ts (3)
dependency(68-76)dependency(77-77)cssPath(26-93)packages/uniwind/src/metro/metro-css-patches.ts (1)
file(42-42)packages/uniwind/tests/setup.ts (1)
cssPath(6-23)
packages/uniwind/tests/media-queries/color-scheme.test.css (2)
packages/uniwind/tests/styles-parsing/colors.test.tsx (1)
test(5-84)packages/uniwind/src/core/config/config.ts (1)
updateCSSVariables(32-50)
packages/uniwind/src/metro/processor/processor.ts (2)
packages/uniwind/src/metro/types.ts (1)
MediaQueryResolver(29-42)packages/uniwind/src/metro/addMetaToStylesTemplate.ts (1)
Processor(30-152)
packages/uniwind/tests/media-queries/combined.test.tsx (5)
packages/uniwind/src/metro/compileVirtual.ts (1)
compileVirtual(21-74)packages/uniwind/src/components/index.ts (1)
Platform(193-195)packages/uniwind/src/core/native/store.ts (1)
UniwindStore(222-222)packages/uniwind/src/core/listener.ts (1)
UniwindListener(59-59)packages/uniwind/tests/utils.ts (1)
renderUniwind(6-18)
packages/uniwind/tests/media-queries/max-width.test.tsx (5)
packages/uniwind/src/metro/compileVirtual.ts (1)
compileVirtual(21-74)packages/uniwind/src/components/index.ts (1)
Platform(193-195)packages/uniwind/src/core/native/store.ts (1)
UniwindStore(222-222)packages/uniwind/src/core/listener.ts (1)
UniwindListener(59-59)packages/uniwind/tests/utils.ts (1)
renderUniwind(6-18)
packages/uniwind/tests/media-queries/min-width.test.tsx (5)
packages/uniwind/src/metro/compileVirtual.ts (1)
compileVirtual(21-74)packages/uniwind/src/components/index.ts (1)
Platform(193-195)packages/uniwind/src/core/native/store.ts (1)
UniwindStore(222-222)packages/uniwind/src/core/listener.ts (1)
UniwindListener(59-59)packages/uniwind/tests/utils.ts (1)
renderUniwind(6-18)
🪛 Biome (2.1.2)
packages/uniwind/tests/media-queries/orientation.test.tsx
[error] 26-26: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
packages/uniwind/tests/media-queries/color-scheme.test.tsx
[error] 26-26: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
packages/uniwind/tests/media-queries/combined.test.tsx
[error] 25-25: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
packages/uniwind/tests/media-queries/max-width.test.tsx
[error] 26-26: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
packages/uniwind/tests/media-queries/min-width.test.tsx
[error] 26-26: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
🔇 Additional comments (17)
packages/uniwind/src/core/native/store.ts (1)
86-86: LGTM!This is a minor refactoring that reorders the
varsdeclaration for better code organization. The variable is still properly initialized before its first use, and the change has no functional impact.packages/uniwind/tests/media-queries/combined.test.css (1)
1-30: LGTM!This test file properly validates combined media query conditions. The CSS variables are correctly defined in
:rootand overridden in media queries that combine multiple conditions (width + orientation, width + color-scheme). The utility classes correctly reference the variables usingvar().packages/uniwind/tests/media-queries/max-width.test.css (1)
1-30: LGTM!The test file correctly validates max-width media queries. The CSS variables are properly defined with responsive overrides at the 480px breakpoint, and the utility classes correctly reference them.
packages/uniwind/tests/media-queries/orientation.test.css (1)
1-25: LGTM!The test file correctly validates orientation-based media queries. The CSS variables are properly overridden for landscape orientation, and the utility classes correctly reference them.
packages/uniwind/tests/media-queries/color-scheme.test.css (1)
1-19: LGTM!The test file correctly validates color-scheme media queries. The CSS variable is properly overridden for dark mode, and the utility class correctly references it.
packages/uniwind/src/metro/types.ts (1)
30-31: Excellent type safety improvement!Changing
maxWidthandminWidthfromanytonumber | nullmakes the types more explicit and prevents potential type errors. This aligns well with the media query processing logic and improves code maintainability.packages/uniwind/tests/media-queries/min-width.test.css (1)
35-53: LGTM!The utility classes correctly reference the CSS variables using
var(). They will work as expected once the media query ordering issue is resolved.packages/uniwind/tests/media-queries/color-scheme.test.tsx (2)
26-30: eval() usage is acceptable in test context.The static analysis tool flagged the use of
eval()as a security risk. However, this is test code whereeval()is being used to dynamically initialize the Uniwind runtime with compiled CSS. This is a standard pattern in the test suite (as seen inpackages/uniwind/tests/setup.ts).
41-83: Excellent test coverage!The test suite comprehensively validates color-scheme media queries:
- Default light color scheme behavior
- Dark color scheme variable override
- Dynamic updates when color scheme changes
The tests properly manipulate runtime state and use
act()for React updates. Well done!packages/uniwind/src/metro/processor/mq.ts (2)
9-24: LGTM! Improved iteration handles all media queries correctly.The refactored
processMediaQueriesnow properly iterates through all media queries usingfor-ofand accumulates results instead of returning early on first match. This correctly supports scenarios with multiple media conditions (e.g., combined min-width + orientation).
26-40: Recursive condition processing handles nested media queries well.The
processConditionmethod correctly handles:
operationtype conditions by recursively processing nested conditionsfeaturetype conditions by delegating to width/plain query handlersOne minor consideration: the
operationtype can have anoperatorproperty (and/or/not) that isn't currently being considered. Fororconditions, the current behavior of accumulating all values may not match CSS semantics where only one branch should apply. Verify this is intentional for the native use case.packages/uniwind/tests/media-queries/orientation.test.tsx (1)
39-61: LGTM! Good coverage for orientation-based variable resolution.Tests properly verify:
- Default values in portrait orientation
- Overridden values when orientation is landscape
- Correct font-size values (16 vs 40) demonstrate the media query variable resolution works
packages/uniwind/tests/media-queries/combined.test.tsx (1)
40-120: LGTM! Comprehensive combined media query tests.Excellent coverage for AND-combined media queries:
- Both conditions met → applies media query value
- Only one condition met → uses default
- Complex scenario with multiple elements under all conditions
The tests properly verify the intersection behavior expected for combined
@mediarules.packages/uniwind/tests/media-queries/min-width.test.tsx (1)
39-187: LGTM! Excellent comprehensive test coverage for min-width media queries.The test suite covers:
- Default values below thresholds
- Media query values at and above thresholds
- Exact breakpoint boundary behavior
- Highest matching breakpoint selection (cascade)
- Multiple variables with same/different breakpoints
- Dynamic updates across multiple breakpoints
- Edge cases (very small/large widths)
This thoroughly exercises the min-width media query variable resolution.
packages/uniwind/src/metro/processor/processor.ts (3)
24-25: LGTM! New tracking structures for media query variable handling.The
varsWithMediaQueriesmap collects variable declarations that have media query constraints, andpendingVarReferencestracks which classes reference these variables for deferred processing. This design allows media query styles to be correctly associated with utility classes.
136-155: LGTM! Clean declaration parsing with proper type handling.The
parseDeclarationmethod correctly normalizes three declaration shapes:
unparsed: extracts property frompropertyIdcustom: handles CSS custom properties- Default: standard properties
This ensures consistent property/value extraction regardless of how LightningCSS categorizes the declaration.
157-172: VerifyapplyPendingVarReferencesidempotency.This method is called at multiple points in
parseRuleRec(lines 197, 247, 300). Without thedeletecall (commented out on line 171), the same var styles could be pushed multiple times if the sameclassNametriggers multiple apply calls.Consider either:
- Uncommenting the delete to ensure one-time application
- Adding a guard to prevent duplicate pushes
- Documenting why duplicates are acceptable
#!/bin/bash # Check if there are any tests that would catch duplicate style entries rg -n "stylesheets\[" packages/uniwind/src --type ts -A 3 -B 1
|
@Brentlok based on the discussion in #263, I've have created a PR for it. Kindly review as a lot of work went into the refactoring as to how variables are processed for media queries. I realised the only file I needed to work on was This PR:
|
- Corrects the max-width media query test to properly reflect padding changes when screen width crosses the threshold. - Also reorders and deduplicates the min-width media query for --text-base in the CSS file.
@import 'tailwindcss';
@import 'uniwind';
@layer theme {
:root {
--text-base: 16px;
}
}
@media (min-width: 1px) {
:root {
--text-base: 100px;
}
}<View className="flex-1 justify-center items-center">
<Text className="text-base">
Uniwind test
</Text>
</View>
It's not really working even for a simple case like that |
|
@Brentlok Issue resolved, Please I'll need your review Update:
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/uniwind/src/metro/processor/processor.ts (1)
273-283: Fix media query context loss for multiple nested rules in @media blocks.The
declarationConfigreset at line 279 occurs inside theforEachloop, after each nested rule is processed. SincemediaQueriesare pushed at line 276 before the loop, the reset clearsmediaQueriesfromdeclarationConfigfor each subsequent rule.This causes the second and later rules within an
@mediablock to lose the media query context. For example:@media (min-width: 640px) { .class-a { ... } .class-b { ... } }Only
.class-areceives the media query constraints;.class-bprocesses with an emptymediaQueriesarray.Move the
declarationConfigreset outside theforEachloop to preserve media queries across all nested rules.
🧹 Nitpick comments (3)
packages/uniwind/tests/media-queries/variables.test.tsx (1)
26-30: Consider alternatives toeval()for test setup.The static analysis tool correctly flags
eval()as a security risk. While this is contained within a test file and the input is controlled, usingeval()can make debugging harder and sets a precedent that might be copied elsewhere.Consider using
vm.runInThisContext()from Node'svmmodule or restructuring the test setup to avoid dynamic code evaluation:🔎 Alternative approach using vm module
+import { runInThisContext } from 'vm' import { act } from '@testing-library/react-native' import { readFileSync } from 'fs' ... - eval( - `const { Uniwind } = require('../../src/core/config/config.native'); - Uniwind.__reinit(rt => ${virtualCode}, ['light', 'dark']); - `, - ) + const { Uniwind } = require('../../src/core/config/config.native') + const initFn = runInThisContext(`(rt => ${virtualCode})`) + Uniwind.__reinit(initFn, ['light', 'dark'])packages/uniwind/src/metro/processor/processor.ts (2)
24-25: Consider stronger typing for the new state trackers.Both
varsWithMediaQueriesand the values inpendingVarReferencesuseanyor loose array types. While this follows existing patterns in the file (e.g., line 12), adding explicit types would improve maintainability and catch errors earlier.
43-56: Add defensive check forstylesheets[className]access.Line 51 uses a non-null assertion (
stylesheets[className]!.push). While the current flow should guarantee the stylesheet exists (set at line 189-190 beforeaddDeclarationis called), a defensive check would prevent runtime errors if the code path changes in the future.🔎 Suggested defensive check
for (const varStyle of varStyles) { - this.stylesheets[className]!.push(varStyle) + if (this.stylesheets[className]) { + this.stylesheets[className].push(varStyle) + } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/uniwind/src/metro/processor/processor.tspackages/uniwind/tests/media-queries/variables.test.csspackages/uniwind/tests/media-queries/variables.test.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/uniwind/tests/media-queries/variables.test.css
🧰 Additional context used
🧬 Code graph analysis (2)
packages/uniwind/src/metro/processor/processor.ts (4)
packages/uniwind/src/metro/types.ts (1)
MediaQueryResolver(29-42)packages/uniwind/src/metro/addMetaToStylesTemplate.ts (2)
Processor(30-152)style(33-135)packages/uniwind/src/core/web/cssListener.ts (1)
addMediaQueriesDeep(142-160)packages/uniwind/src/core/native/store.ts (1)
resolveStyles(83-220)
packages/uniwind/tests/media-queries/variables.test.tsx (5)
packages/uniwind/src/metro/compileVirtual.ts (1)
compileVirtual(21-74)packages/uniwind/src/components/index.ts (1)
Platform(193-195)packages/uniwind/src/core/native/store.ts (1)
UniwindStore(223-223)packages/uniwind/src/core/listener.ts (1)
UniwindListener(59-59)packages/uniwind/tests/utils.ts (1)
renderUniwind(6-18)
🪛 Biome (2.1.2)
packages/uniwind/tests/media-queries/variables.test.tsx
[error] 26-26: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
🔇 Additional comments (6)
packages/uniwind/tests/media-queries/variables.test.tsx (2)
39-86: LGTM! Comprehensive min-width media query tests.Good coverage of edge cases including threshold boundary testing and multiple variable handling. The test at line 73 intentionally omits
UniwindListener.notify()which validates that screen dimension changes alone are sufficient when accessed at render time.
88-132: LGTM! Dynamic update tests properly useact()for state changes.The tests correctly wrap
UniwindListener.notify()calls inact()to ensure React processes the state updates synchronously before assertions. The asymmetric setup (one test uses notify before render, the other usesreinit()) provides good coverage of different initialization paths.packages/uniwind/src/metro/processor/processor.ts (4)
75-77: LGTM! Helper correctly identifies media query presence.The method checks the relevant CSS media query features (width constraints, orientation, color-scheme) while correctly excluding pseudo-class conditions (active, focus, etc.) which are handled separately.
95-99: LGTM! Deferred variable storage for media queries.Correctly stores media-query-scoped CSS variables in a separate structure for later application to classes that reference them. Each variant gets its own style object.
130-149: LGTM! Variable reference tracking logic.The regex correctly matches the compiled variable reference pattern. The deduplication check at line 146 prevents redundant entries, and the non-null assertion at line 144 is safe since a successful regex match guarantees the capture group exists.
152-171: LGTM! Clean declaration parsing.The helper provides a clean abstraction for extracting property/value pairs from the three declaration types (unparsed, custom, regular), using
CSS.processValueconsistently.
|
@Brentlok I benchmarked the changes for performance, and it's slightly faster than 1.2.2
|
In the discussion I have suggested a different solution, we didn't really discussed something like that but I guess that this idea seems better, well done
That shouldn't really happen because you didn't touch any runtime code - so the benchmark should stay the same. It would be worth investigating how the metadata (metro generated js) looks like on the main branch and in your PR for this benchmark. Because it was random better timing (your computer was doing less work or smth) or some metadata has changed and something may not work. Because it is pretty huge PR and as I mentioned before it doesn't have really high priority atm - we can't really review it right now. We're focused on the pro version right now, and this change looks pretty risky to merge without major testing (our unit tests are not perfect yet). We'll get back to this when we're done with the other staff, again thanks for contribution ✌️ |
2 participants
fixes #242
Summary by CodeRabbit
New Features
Tests
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.