Skip to content

Commit 8ed77c2

Browse files
mattcompilesmattcompiles
authoredNov 16, 2022
Fix conditional style ordering (#893)
·
@vanilla-extract/webpack-plugin@2.3.18@vanilla-extract/babel-plugin-debug-ids@1.0.1
1 parent 176c026 commit 8ed77c2

File tree

3 files changed

+236
-49
lines changed

3 files changed

+236
-49
lines changed
 

‎.changeset/smooth-masks-mate.md

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@vanilla-extract/css': patch
3+
---
4+
5+
Fix issue where conditional styles (e.g. `@media`, `@supports`, etc) could be ordered incorrectly

‎packages/css/src/conditionalRulesets.ts

+46-42
Original file line numberDiff line numberDiff line change
@@ -1,32 +1,34 @@
1+
/** e.g. @media screen and (min-width: 500px) */
2+
type Query = string;
3+
14
interface Rule {
25
selector: string;
36
rule: any;
47
}
58

69
type Condition = {
7-
query: string;
10+
query: Query;
811
rules: Array<Rule>;
912
children: ConditionalRuleset;
1013
};
1114

1215
export class ConditionalRuleset {
13-
ruleset: Array<Condition>;
16+
ruleset: Map<Query, Condition>;
17+
1418
/**
15-
* Stores information about where conditions must in relation to other conditions
19+
* Stores information about where conditions must be in relation to other conditions
1620
*
1721
* e.g. mobile -> tablet, desktop
1822
*/
19-
precedenceLookup: Map<string, Set<String>>;
23+
precedenceLookup: Map<Query, Set<String>>;
2024

2125
constructor() {
22-
this.ruleset = [];
26+
this.ruleset = new Map();
2327
this.precedenceLookup = new Map();
2428
}
2529

26-
findOrCreateCondition(conditionQuery: string) {
27-
let targetCondition = this.ruleset.find(
28-
(cond) => cond.query === conditionQuery,
29-
);
30+
findOrCreateCondition(conditionQuery: Query) {
31+
let targetCondition = this.ruleset.get(conditionQuery);
3032

3133
if (!targetCondition) {
3234
// No target condition so create one
@@ -35,13 +37,13 @@ export class ConditionalRuleset {
3537
rules: [],
3638
children: new ConditionalRuleset(),
3739
};
38-
this.ruleset.push(targetCondition);
40+
this.ruleset.set(conditionQuery, targetCondition);
3941
}
4042

4143
return targetCondition;
4244
}
4345

44-
getConditionalRulesetByPath(conditionPath: Array<string>) {
46+
getConditionalRulesetByPath(conditionPath: Array<Query>) {
4547
let currRuleset: ConditionalRuleset = this;
4648

4749
for (const query of conditionPath) {
@@ -53,7 +55,7 @@ export class ConditionalRuleset {
5355
return currRuleset;
5456
}
5557

56-
addRule(rule: Rule, conditionQuery: string, conditionPath: Array<string>) {
58+
addRule(rule: Rule, conditionQuery: Query, conditionPath: Array<Query>) {
5759
const ruleset = this.getConditionalRulesetByPath(conditionPath);
5860
const targetCondition = ruleset.findOrCreateCondition(conditionQuery);
5961

@@ -65,22 +67,22 @@ export class ConditionalRuleset {
6567
}
6668

6769
addConditionPrecedence(
68-
conditionPath: Array<string>,
69-
conditionOrder: Array<string>,
70+
conditionPath: Array<Query>,
71+
conditionOrder: Array<Query>,
7072
) {
7173
const ruleset = this.getConditionalRulesetByPath(conditionPath);
7274

7375
for (let i = 0; i < conditionOrder.length; i++) {
74-
const condition = conditionOrder[i];
76+
const query = conditionOrder[i];
7577

7678
const conditionPrecedence =
77-
ruleset.precedenceLookup.get(condition) ?? new Set();
79+
ruleset.precedenceLookup.get(query) ?? new Set();
7880

7981
for (const lowerPrecedenceCondition of conditionOrder.slice(i + 1)) {
8082
conditionPrecedence.add(lowerPrecedenceCondition);
8183
}
8284

83-
ruleset.precedenceLookup.set(condition, conditionPrecedence);
85+
ruleset.precedenceLookup.set(query, conditionPrecedence);
8486
}
8587
}
8688

@@ -101,10 +103,8 @@ export class ConditionalRuleset {
101103
}
102104

103105
// Check that children are compatible
104-
for (const { query, children } of incomingRuleset.ruleset) {
105-
const matchingCondition = this.ruleset.find(
106-
(cond) => cond.query === query,
107-
);
106+
for (const { query, children } of incomingRuleset.ruleset.values()) {
107+
const matchingCondition = this.ruleset.get(query);
108108

109109
if (
110110
matchingCondition &&
@@ -119,17 +119,15 @@ export class ConditionalRuleset {
119119

120120
merge(incomingRuleset: ConditionalRuleset) {
121121
// Merge rulesets into one array
122-
for (const { query, rules, children } of incomingRuleset.ruleset) {
123-
const matchingCondition = this.ruleset.find(
124-
(cond) => cond.query === query,
125-
);
122+
for (const { query, rules, children } of incomingRuleset.ruleset.values()) {
123+
const matchingCondition = this.ruleset.get(query);
126124

127125
if (matchingCondition) {
128126
matchingCondition.rules.push(...rules);
129127

130128
matchingCondition.children.merge(children);
131129
} else {
132-
this.ruleset.push({ query, rules, children });
130+
this.ruleset.set(query, { query, rules, children });
133131
}
134132
}
135133

@@ -162,33 +160,39 @@ export class ConditionalRuleset {
162160
return true;
163161
}
164162

165-
sort() {
166-
this.ruleset.sort((a, b) => {
167-
const aWeights = this.precedenceLookup.get(a.query);
163+
getSortedRuleset() {
164+
const sortedRuleset: Array<Condition> = [];
165+
166+
// Loop through all queries and add them to the sorted ruleset
167+
for (const [query, dependents] of this.precedenceLookup.entries()) {
168+
const conditionForQuery = this.ruleset.get(query);
168169

169-
if (aWeights?.has(b.query)) {
170-
// A is higher precedence
171-
return -1;
170+
if (!conditionForQuery) {
171+
throw new Error(`Can't find condition for ${query}`);
172172
}
173173

174-
const bWeights = this.precedenceLookup.get(b.query);
174+
// Find the location of the first dependent condition in the sortedRuleset
175+
// A dependent condition is a condition that must be placed *after* the current one
176+
const firstMatchingDependent = sortedRuleset.findIndex((condition) =>
177+
dependents.has(condition.query),
178+
);
175179

176-
if (bWeights?.has(a.query)) {
177-
// B is higher precedence
178-
return 1;
180+
if (firstMatchingDependent > -1) {
181+
// Insert the condition before the dependent one
182+
sortedRuleset.splice(firstMatchingDependent, 0, conditionForQuery);
183+
} else {
184+
// No match, just insert at the end
185+
sortedRuleset.push(conditionForQuery);
179186
}
187+
}
180188

181-
return 0;
182-
});
189+
return sortedRuleset;
183190
}
184191

185192
renderToArray() {
186-
// Sort rulesets according to required rule order
187-
this.sort();
188-
189193
const arr: any = [];
190194

191-
for (const { query, rules, children } of this.ruleset) {
195+
for (const { query, rules, children } of this.getSortedRuleset()) {
192196
const selectors: any = {};
193197

194198
for (const rule of rules) {

‎packages/css/src/transformCss.test.ts

+185-7
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ describe('transformCss', () => {
158158
`);
159159
});
160160

161-
it('should combine media queries', () => {
161+
it('should merge media queries', () => {
162162
expect(
163163
transformCss({
164164
composedClassLists: [],
@@ -234,7 +234,7 @@ describe('transformCss', () => {
234234
`);
235235
});
236236

237-
it('should not combine media queries if not safe to do so', () => {
237+
it('should not merge media queries if not safe to do so', () => {
238238
expect(
239239
transformCss({
240240
composedClassLists: [],
@@ -344,7 +344,7 @@ describe('transformCss', () => {
344344
`);
345345
});
346346

347-
it('should not combine nested media queries if not safe to do so', () => {
347+
it('should not merge nested media queries if not safe to do so', () => {
348348
expect(
349349
transformCss({
350350
composedClassLists: [],
@@ -474,6 +474,75 @@ describe('transformCss', () => {
474474
`);
475475
});
476476

477+
it('should merge nested media queries if safe to do so', () => {
478+
expect(
479+
transformCss({
480+
composedClassLists: [],
481+
localClassNames: ['testClass'],
482+
cssObjs: [
483+
{
484+
type: 'local',
485+
selector: 'testClass',
486+
rule: {
487+
'@media': {
488+
'screen and (min-width: 1024px)': {
489+
padding: '50px',
490+
},
491+
},
492+
},
493+
},
494+
495+
{
496+
type: 'local',
497+
selector: '.otherClass',
498+
rule: {
499+
'@media': {
500+
'screen and (min-width: 1200px)': {
501+
color: 'red',
502+
},
503+
},
504+
},
505+
},
506+
507+
{
508+
type: 'local',
509+
selector: '.otherOtherClass',
510+
rule: {
511+
'@media': {
512+
'screen and (min-width: 768px)': {
513+
background: 'blue',
514+
},
515+
516+
'screen and (min-width: 1024px)': {
517+
background: 'green',
518+
},
519+
},
520+
},
521+
},
522+
],
523+
}).join('\n'),
524+
).toMatchInlineSnapshot(`
525+
"@media screen and (min-width: 768px) {
526+
.otherOtherClass {
527+
background: blue;
528+
}
529+
}
530+
@media screen and (min-width: 1024px) {
531+
.testClass {
532+
padding: 50px;
533+
}
534+
.otherOtherClass {
535+
background: green;
536+
}
537+
}
538+
@media screen and (min-width: 1200px) {
539+
.otherClass {
540+
color: red;
541+
}
542+
}"
543+
`);
544+
});
545+
477546
it('should handle simple pseudos', () => {
478547
expect(
479548
transformCss({
@@ -1081,7 +1150,7 @@ describe('transformCss', () => {
10811150
`);
10821151
});
10831152

1084-
it('should merge nested @supports, @media and @container queries', () => {
1153+
it('should merge nested @supports, @media and @container queries when safe to do so', () => {
10851154
expect(
10861155
transformCss({
10871156
composedClassLists: [],
@@ -1100,14 +1169,17 @@ describe('transformCss', () => {
11001169
'sidebar (min-width: 700px)': {
11011170
display: 'grid',
11021171
},
1172+
1173+
'sidebar (min-width: 800px)': {
1174+
display: 'grid',
1175+
},
11031176
},
11041177
},
11051178
},
11061179
},
11071180
},
11081181
},
11091182
},
1110-
11111183
{
11121184
type: 'local',
11131185
selector: 'otherClass',
@@ -1116,11 +1188,15 @@ describe('transformCss', () => {
11161188
'screen and (min-width: 700px)': {
11171189
'@supports': {
11181190
'(display: grid)': {
1119-
backgroundColor: 'yellow',
1191+
borderColor: 'blue',
11201192
'@container': {
11211193
'sidebar (min-width: 700px)': {
11221194
display: 'grid',
11231195
},
1196+
1197+
'sidebar (min-width: 800px)': {
1198+
display: 'grid',
1199+
},
11241200
},
11251201
},
11261202
},
@@ -1137,7 +1213,7 @@ describe('transformCss', () => {
11371213
border-color: blue;
11381214
}
11391215
.otherClass {
1140-
background-color: yellow;
1216+
border-color: blue;
11411217
}
11421218
@container sidebar (min-width: 700px) {
11431219
.testClass {
@@ -1147,6 +1223,108 @@ describe('transformCss', () => {
11471223
display: grid;
11481224
}
11491225
}
1226+
@container sidebar (min-width: 800px) {
1227+
.testClass {
1228+
display: grid;
1229+
}
1230+
.otherClass {
1231+
display: grid;
1232+
}
1233+
}
1234+
}
1235+
}"
1236+
`);
1237+
});
1238+
1239+
it('should not merge nested @supports, @media and @container queries when not safe to do so', () => {
1240+
expect(
1241+
transformCss({
1242+
composedClassLists: [],
1243+
localClassNames: ['testClass', 'otherClass'],
1244+
cssObjs: [
1245+
{
1246+
type: 'local',
1247+
selector: 'testClass',
1248+
rule: {
1249+
'@media': {
1250+
'screen and (min-width: 700px)': {
1251+
'@supports': {
1252+
'(display: grid)': {
1253+
borderColor: 'blue',
1254+
'@container': {
1255+
'sidebar (min-width: 700px)': {
1256+
display: 'grid',
1257+
},
1258+
1259+
'sidebar (min-width: 800px)': {
1260+
display: 'grid',
1261+
},
1262+
},
1263+
},
1264+
},
1265+
},
1266+
},
1267+
},
1268+
},
1269+
{
1270+
type: 'local',
1271+
selector: 'otherClass',
1272+
rule: {
1273+
'@media': {
1274+
'screen and (min-width: 700px)': {
1275+
'@supports': {
1276+
'(display: grid)': {
1277+
borderColor: 'blue',
1278+
'@container': {
1279+
'sidebar (min-width: 800px)': {
1280+
display: 'grid',
1281+
},
1282+
1283+
'sidebar (min-width: 700px)': {
1284+
display: 'grid',
1285+
},
1286+
},
1287+
},
1288+
},
1289+
},
1290+
},
1291+
},
1292+
},
1293+
],
1294+
}).join('\n'),
1295+
).toMatchInlineSnapshot(`
1296+
"@media screen and (min-width: 700px) {
1297+
@supports (display: grid) {
1298+
.testClass {
1299+
border-color: blue;
1300+
}
1301+
@container sidebar (min-width: 700px) {
1302+
.testClass {
1303+
display: grid;
1304+
}
1305+
}
1306+
@container sidebar (min-width: 800px) {
1307+
.testClass {
1308+
display: grid;
1309+
}
1310+
}
1311+
}
1312+
}
1313+
@media screen and (min-width: 700px) {
1314+
@supports (display: grid) {
1315+
.otherClass {
1316+
border-color: blue;
1317+
}
1318+
@container sidebar (min-width: 800px) {
1319+
.otherClass {
1320+
display: grid;
1321+
}
1322+
}
1323+
@container sidebar (min-width: 700px) {
1324+
.otherClass {
1325+
display: grid;
1326+
}
1327+
}
11501328
}
11511329
}"
11521330
`);

0 commit comments

Comments
 (0)
Please sign in to comment.