Skip to content

Commit 42e45a4

Browse files
MadLittleModsMadLittleMods
committed
Fix dynimically added variable usage from expanding causing extra expansion
See `npm test -- --grep="variable-reference-other-variable-media-query2"`
1 parent 71950b8 commit 42e45a4

10 files changed

+207
-100
lines changed

‎.eslintrc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
"es6": true,
44
"node": true,
55
"browser": true,
6+
"mocha": true,
67
},
78
"parserOptions": {
89
"ecmaVersion": 6,

‎index.js

Lines changed: 29 additions & 91 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,13 @@ const specificityLib = require('specificity');
44
const generateSelectorBranchesFromPostcssNode = require('postcss-node-scope-utility/lib/generate-branches');
55
const isSelectorBranchUnderScope = require('postcss-node-scope-utility/lib/is-branch-under-scope');
66

7-
// A custom property is any property whose name starts with two dashes (U+002D HYPHEN-MINUS)
8-
// `--foo`
9-
// See: http://dev.w3.org/csswg/css-variables/#custom-property
10-
const RE_VAR_PROP = (/(--(.+))/);
11-
const RE_VAR_FUNC = (/var\((--[^,\s]+?)(?:\s*,\s*(.+))?\)/);
7+
const {
8+
RE_VAR_PROP,
9+
RE_VAR_FUNC
10+
} = require('./lib/var-regex');
11+
const expandVarUsageFromVarDefinition = require('./lib/expand-var-usage-from-var-definition');
12+
13+
1214

1315
function getSpecificity(selector) {
1416
// We only care about the first piece because we have already split the comma-separated pieces before we use this
@@ -33,30 +35,6 @@ function eachCssVariableDeclaration(css, cb) {
3335
});
3436
}
3537

36-
function cloneParentAncestry(node) {
37-
const clone = node.clone();
38-
clone.removeAll();
39-
40-
if(node.parent && node.parent.type !== 'root') {
41-
const parentClone = node.parent.clone();
42-
parentClone.removeAll();
43-
parentClone.append(clone);
44-
45-
return cloneParentAncestry(parentClone);
46-
}
47-
48-
return clone;
49-
}
50-
51-
function insertNodeIntoAncestry(ancestry, insertNode) {
52-
let deepestNode = ancestry;
53-
while(!deepestNode.nodes || deepestNode.nodes.length > 0) {
54-
deepestNode = deepestNode.nodes[0];
55-
}
56-
57-
deepestNode.append(insertNode);
58-
}
59-
6038

6139
var defaults = {
6240
// Allows you to preserve custom properties & var() usage in output.
@@ -117,22 +95,25 @@ module.exports = postcss.plugin('postcss-css-variables', function(options) {
11795
// Collect all of the variables defined `--foo: #f00;`
11896
// ---------------------------------------------------------
11997
// ---------------------------------------------------------
98+
const usageDeclsToSkip = [];
12099
eachCssVariableDeclaration(css, function(variableDecl) {
121100
// We cache the parent rule because after decl removal, it will be undefined
122101
const variableDeclParentRule = variableDecl.parent;
123102
const variableName = variableDecl.prop;
124103
const variableValue = variableDecl.value;
125104
const isImportant = variableDecl.important || false;
126-
const variableSelectorBranchs = generateSelectorBranchesFromPostcssNode(variableDeclParentRule);
105+
const variableSelectorBranches = generateSelectorBranchesFromPostcssNode(variableDeclParentRule);
127106

128107
debug(`Collecting ${variableName}=${variableValue} isImportant=${isImportant} from ${variableDeclParentRule.selector.toString()}`);
129108

130-
map[variableName] = (map[variableName] || []).concat({
109+
const variableEntry = {
131110
name: variableName,
132111
value: variableValue,
133112
isImportant,
134-
selectorBranches: variableSelectorBranchs
135-
});
113+
selectorBranches: variableSelectorBranches
114+
};
115+
116+
map[variableName] = (map[variableName] || []).concat(variableEntry);
136117

137118

138119
// Expand/rollout/unroll variable usage
@@ -146,69 +127,26 @@ module.exports = postcss.plugin('postcss-css-variables', function(options) {
146127
// .foo:hover { --color: #0f0; color: var(--color); }
147128
// --------------------------------
148129
css.walkDecls(function(usageDecl) {
149-
// Avoid duplicating the usage decl on itself
150-
// And make sure this decl has `var()` usage
151-
if(variableDeclParentRule === usageDecl.parent || !RE_VAR_FUNC.test(usageDecl.value)) {
130+
if(
131+
// Avoid iterating a var declaration `--var: #f00`
132+
RE_VAR_PROP.test(usageDecl.prop) ||
133+
// Make sure this decl has `var()` usage
134+
!RE_VAR_FUNC.test(usageDecl.value) ||
135+
// Avoid duplicating the usage decl on itself
136+
variableDeclParentRule === usageDecl.parent ||
137+
// Avoid iterating a clone we may have just added before
138+
usageDeclsToSkip.some((skipDecl) => skipDecl === usageDecl)
139+
) {
152140
return;
153141
}
154142

155-
const usageSelectorBranches = generateSelectorBranchesFromPostcssNode(usageDecl.parent);
156-
157-
variableSelectorBranchs.some((variableSelectorBranch) => {
158-
return usageSelectorBranches.some((usageSelectorBranch) => {
159-
// In this case, we look whether the usage is under the scope of the definition
160-
const isUnderScope = isSelectorBranchUnderScope(usageSelectorBranch, variableSelectorBranch);
161-
162-
debug(`Should unroll decl? isUnderScope=${isUnderScope}`, usageSelectorBranch.selector.toString(), '|', variableSelectorBranch.selector.toString())
163-
164-
// For general cases, we can put variable usage right-below the variable definition
165-
if(isUnderScope) {
166-
usageDecl.value.replace(new RegExp(RE_VAR_FUNC.source, 'g'), (match, matchedVariableName) => {
167-
if(matchedVariableName === variableName) {
168-
variableDecl.after(usageDecl.clone());
169-
}
170-
});
171-
}
172-
// For at-rules
173-
else {
174-
// If there is a conditional like a atrule/media-query, then we should check whether
175-
// the variable can apply and put our usage within that same context
176-
// Before:
177-
// :root { --color: #f00; }
178-
// @media (max-width: 1000px) { :root { --color: #0f0; } }
179-
// .box { color: var(--color); }
180-
// After:
181-
// .box { color: #f00; }
182-
// @media (max-width: 1000px) {.box { color: #0f0; } }
183-
const hasAtRule = (variableSelectorBranch.conditionals || []).some((conditional) => {
184-
return conditional.type === 'atrule';
185-
})
186-
if(hasAtRule) {
187-
const doesVariableApplyToUsage = isSelectorBranchUnderScope(variableSelectorBranch, usageSelectorBranch, { ignoreConditionals: true });
188-
debug(`Should expand atrule? doesVariableApplyToUsage=${doesVariableApplyToUsage}`, variableSelectorBranch.selector.toString(), '|', usageSelectorBranch.selector.toString())
189-
190-
if(doesVariableApplyToUsage) {
191-
// Create a new usage clone with only the usage decl
192-
const newUsageRule = usageDecl.parent.clone();
193-
newUsageRule.removeAll();
194-
newUsageRule.append(usageDecl.clone());
195-
196-
const variableAncestry = cloneParentAncestry(variableDecl.parent.parent);
197-
insertNodeIntoAncestry(variableAncestry, newUsageRule);
198-
199-
usageDecl.parent.cloneBefore();
200-
usageDecl.parent.replaceWith(variableAncestry);
201-
}
202-
}
203-
}
204-
205-
206-
return isUnderScope;
207-
});
208-
});
143+
const newUsageDecl = expandVarUsageFromVarDefinition(variableEntry, variableDecl, usageDecl);
144+
// Keep track of the cloned decls we should skip over
145+
usageDeclsToSkip.push(newUsageDecl);
209146
});
210147

211148

149+
212150
// Remove the variable declaration because they are pretty much useless after we resolve them
213151
if(!opts.preserve) {
214152
variableDecl.remove();
@@ -270,7 +208,7 @@ module.exports = postcss.plugin('postcss-css-variables', function(options) {
270208
const isUnderScope = isSelectorBranchUnderScope(variableSelectorBranch, selectorBranch);
271209
const specificity = getSpecificity(variableSelectorBranch.selector.toString());
272210

273-
debug(`isUnderScope=${isUnderScope} compareSpecificity=${compareSpecificity(specificity, currentGreatestSpecificity)} specificity=${specificity}`, variableSelectorBranch.selector.toString(), '|', selectorBranch.selector.toString())
211+
debug(`isUnderScope=${isUnderScope} compareSpecificity=${compareSpecificity(specificity, currentGreatestSpecificity)} specificity=${specificity}`, variableSelectorBranch.selector.toString(), '|', selectorBranch.selector.toString(), '-->', variableEntry.value)
274212

275213
if(isUnderScope && compareSpecificity(specificity, currentGreatestSpecificity) >= 0) {
276214
currentGreatestSpecificity = specificity;

‎lib/clone-parent-ancestry.js

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
function cloneParentAncestry(node) {
2+
let ancestry = null;
3+
4+
let currentNode = node;
5+
while(currentNode.parent && currentNode.parent.type !== 'root') {
6+
const parentClone = currentNode.parent.clone();
7+
parentClone.removeAll();
8+
9+
if(ancestry) {
10+
parentClone.append(ancestry);
11+
}
12+
13+
ancestry = parentClone;
14+
currentNode = currentNode.parent;
15+
}
16+
17+
return ancestry;
18+
}
19+
20+
module.exports = cloneParentAncestry;

‎lib/expand-var-usage-from-var-definition.js

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
const debug = require('debug')('postcss-css-variables:plugin:expand-var-usage');
2+
const generateSelectorBranchesFromPostcssNode = require('postcss-node-scope-utility/lib/generate-branches');
3+
const isSelectorBranchUnderScope = require('postcss-node-scope-utility/lib/is-branch-under-scope');
4+
5+
const {
6+
RE_VAR_FUNC
7+
} = require('./var-regex');
8+
const cloneParentAncestry = require('./clone-parent-ancestry');
9+
10+
function insertNodeIntoAncestry(ancestry, insertNode) {
11+
let deepestNode = ancestry;
12+
while(!deepestNode.nodes || deepestNode.nodes.length > 0) {
13+
deepestNode = deepestNode.nodes[0];
14+
}
15+
16+
deepestNode.append(insertNode);
17+
}
18+
19+
20+
function expandVarUsageFromVarDefinition(variableEntry, variableDecl, usageDecl) {
21+
const newUsageDecl = usageDecl.clone();
22+
const usageSelectorBranches = generateSelectorBranchesFromPostcssNode(usageDecl.parent);
23+
24+
variableEntry.selectorBranches.some((variableSelectorBranch) => {
25+
return usageSelectorBranches.some((usageSelectorBranch) => {
26+
// In this case, we look whether the usage is under the scope of the definition
27+
const isUnderScope = isSelectorBranchUnderScope(usageSelectorBranch, variableSelectorBranch);
28+
29+
debug(`Should unroll decl? isUnderScope=${isUnderScope}`, usageSelectorBranch.selector.toString(), '|', variableSelectorBranch.selector.toString())
30+
31+
// For general cases, we can put variable usage right-below the variable definition
32+
if(isUnderScope) {
33+
usageDecl.value.replace(new RegExp(RE_VAR_FUNC.source, 'g'), (match, matchedVariableName) => {
34+
if(matchedVariableName === variableEntry.name) {
35+
variableDecl.after(newUsageDecl);
36+
}
37+
});
38+
}
39+
// For at-rules
40+
else {
41+
// If there is a conditional like a atrule/media-query, then we should check whether
42+
// the variable can apply and put our usage within that same context
43+
// Before:
44+
// :root { --color: #f00; }
45+
// @media (max-width: 1000px) { :root { --color: #0f0; } }
46+
// .box { color: var(--color); }
47+
// After:
48+
// .box { color: #f00; }
49+
// @media (max-width: 1000px) {.box { color: #0f0; } }
50+
const hasAtRule = (variableSelectorBranch.conditionals || []).some((conditional) => {
51+
return conditional.type === 'atrule';
52+
})
53+
if(hasAtRule) {
54+
const doesVariableApplyToUsage = isSelectorBranchUnderScope(variableSelectorBranch, usageSelectorBranch, { ignoreConditionals: true });
55+
debug(`Should expand atrule? doesVariableApplyToUsage=${doesVariableApplyToUsage}`, variableSelectorBranch.selector.toString(), '|', usageSelectorBranch.selector.toString())
56+
57+
if(doesVariableApplyToUsage) {
58+
// Create a new usage clone with only the usage decl
59+
const newUsageRule = usageDecl.parent.clone();
60+
newUsageRule.removeAll();
61+
newUsageRule.append(newUsageDecl);
62+
63+
const variableAncestry = cloneParentAncestry(variableDecl.parent);
64+
insertNodeIntoAncestry(variableAncestry, newUsageRule);
65+
66+
usageDecl.parent.cloneBefore();
67+
usageDecl.parent.replaceWith(variableAncestry);
68+
}
69+
}
70+
}
71+
72+
return isUnderScope;
73+
});
74+
});
75+
76+
return newUsageDecl;
77+
}
78+
79+
module.exports = expandVarUsageFromVarDefinition;

‎lib/var-regex.js

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
// A custom property is any property whose name starts with two dashes (U+002D HYPHEN-MINUS)
2+
// `--foo`
3+
// See: http://dev.w3.org/csswg/css-variables/#custom-property
4+
const RE_VAR_PROP = (/(--(.+))/);
5+
const RE_VAR_FUNC = (/var\((--[^,\s]+?)(?:\s*,\s*(.+))?\)/);
6+
7+
module.exports = {
8+
RE_VAR_PROP,
9+
RE_VAR_FUNC
10+
};

‎test/clone-parent-ancestry-test.js

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
2+
var chai = require('chai');
3+
var expect = chai.expect;
4+
var chaiAsPromised = require('chai-as-promised');
5+
chai.use(chaiAsPromised);
6+
7+
var postcss = require('postcss');
8+
var cloneParentAncestry = require('../lib/clone-parent-ancestry');
9+
10+
let getDeepestChildRule = function(css) {
11+
let deepestNode;
12+
let currentNode = postcss.parse(css);
13+
while(currentNode) {
14+
//console.log(currentNode);
15+
let child = currentNode.first;
16+
if(child && child.type === 'rule') {
17+
deepestNode = child;
18+
}
19+
20+
currentNode = child;
21+
}
22+
23+
return deepestNode;
24+
};
25+
26+
27+
var test = function(message, css, expected) {
28+
it(message, function() {
29+
const node = getDeepestChildRule(css);
30+
31+
const ancestry = cloneParentAncestry(node);
32+
33+
expect(ancestry && ancestry.toString().replace(/(\r?\n)|\s/g, '')).to.equal(expected);
34+
});
35+
};
36+
37+
38+
describe('cloneParentAncestry', () => {
39+
test('should work with 1 rule', `.foo{}`, null);
40+
41+
test('should work with nested rules', `.foo{.bar{}}`, '.foo{}');
42+
43+
test('should work with nested at-rules and rules', `@media print { .foo {} }`, '@mediaprint{}');
44+
45+
test('should with at-rules', `@media print { .foo { .bar{} } }`, '@mediaprint{.foo{}}');
46+
47+
test('should work with 2 levels of at-rules', `
48+
@media print {
49+
@media (max-width: 1250px) {
50+
.foo {}
51+
}
52+
}
53+
`, '@mediaprint{@media(max-width:1250px){}}');
54+
55+
test('should work with 3 levels of at-rules', `
56+
@media print {
57+
@media (max-width: 1250px) {
58+
@media (max-width: 1000px) {
59+
.foo {}
60+
}
61+
}
62+
}
63+
`, '@mediaprint{@media(max-width:1250px){@media(max-width:1000px){}}}');
64+
});

‎test/fixtures/media-query-nested.css

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,4 +18,4 @@
1818

1919
.box {
2020
width: var(--width);
21-
}
21+
}

‎test/fixtures/media-query-nested.expected.css

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,8 @@
33
}
44

55
@media print {
6-
76
@media (max-width: 1250px) {
8-
97
@media (max-width: 1000px) {
10-
118
.box {
129
width: 450px;
1310
}
@@ -16,11 +13,9 @@
1613
}
1714

1815
@media print {
19-
2016
@media (max-width: 1250px) {
21-
2217
.box {
2318
width: 300px;
2419
}
2520
}
26-
}
21+
}

0 commit comments

Comments
 (0)