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

Ignore map property name (#4375) #4450

Merged
merged 6 commits into from Dec 1, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 26 additions & 0 deletions lib/rules/unit-no-unknown/__tests__/index.js
Expand Up @@ -437,6 +437,19 @@ testRule(rule, {
code: 'a { margin: calc(100% - #{$margin * 2}); }',
description: 'work with interpolation',
},
{
code: '$foo: ( 1prop: 10px, 2prop: 12px, 3prop: /* comment */ 14px )',
description: 'ignore map property name',
},
{
code: '$breakpoints: ( small: /* comment */ 767px, 1medium: 992px, large: 1200px );',
description: 'ignore map property name',
},
{
code:
'$breakpoints: ( small: /* comment */ 767px, medium: ( 1prop: 992px, 2prop: ( 1prop: 1200px ) ) );',
description: 'ignore map property name in nested maps',
},
],

reject: [
Expand Down Expand Up @@ -470,6 +483,19 @@ testRule(rule, {
line: 1,
column: 39,
},
{
code: '$breakpoints: ( small: 767px, 1medium: 992pix, large: 1200px );',
message: messages.rejected('pix'),
line: 1,
column: 40,
},
{
code:
'$breakpoints: ( small: /* comment */ 767px, medium: ( 1prop: 992pix, 2prop: ( 1prop: 1200px ) ) );',
message: messages.rejected('pix'),
line: 1,
column: 49,
},
{
code: 'a { font: (italic bold 10px/8pix) }',
message: messages.rejected('pix'),
Expand Down
16 changes: 16 additions & 0 deletions lib/rules/unit-no-unknown/index.js
Expand Up @@ -4,6 +4,7 @@ const _ = require('lodash');
const atRuleParamIndex = require('../../utils/atRuleParamIndex');
const declarationValueIndex = require('../../utils/declarationValueIndex');
const getUnitFromValueNode = require('../../utils/getUnitFromValueNode');
const isMap = require('../../utils/isMap');
const keywordSets = require('../../reference/keywordSets');
const mediaParser = require('postcss-media-query-parser').default;
const optionsMatches = require('../../utils/optionsMatches');
Expand All @@ -18,6 +19,8 @@ const messages = ruleMessages(ruleName, {
rejected: (unit) => `Unexpected unknown unit "${unit}"`,
});

const mapPropertyNameIndexOffset = 4;
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain what this variable for, please? I don't understand from the following.

Copy link
Member Author

@fanich37 fanich37 Nov 29, 2019

Choose a reason for hiding this comment

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

The map property name (in map cleared from comments & spaces) always has index that being divided by 4 gives remainder equals 0 (e.g. 0, 4, 8 etc).
The naming of this const definitely has to be changed since it's not clear. Any suggestions? Or may be comment would be enough?

Copy link
Member

Choose a reason for hiding this comment

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

I think comment in the code would be enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added comment with explanation.


const rule = function(actual, options) {
return (root, result) => {
const validOptions = validateOptions(
Expand All @@ -43,6 +46,7 @@ const rule = function(actual, options) {
// by postcss-value-parser
value = value.replace(/\*/g, ',');
const parsedValue = valueParser(value);
const ignoredMapProperties = [];

parsedValue.walk(function(valueNode) {
// Ignore wrong units within `url` function
Expand All @@ -55,6 +59,18 @@ const rule = function(actual, options) {
return false;
}

if (isMap(valueNode)) {
valueNode.nodes.forEach((node, index) => {
if (!(index % mapPropertyNameIndexOffset)) {
ignoredMapProperties.push(node.sourceIndex);
}
});
}

if (ignoredMapProperties.includes(valueNode.sourceIndex)) {
return;
}

const unit = getUnitFromValueNode(valueNode);

if (!unit) {
Expand Down
66 changes: 66 additions & 0 deletions lib/utils/__tests__/isMap.test.js
@@ -0,0 +1,66 @@
'use strict';

const isMap = require('../isMap');
const sass = require('postcss-sass');
const scss = require('postcss-scss');
const valueParser = require('postcss-value-parser');

describe('isMap', () => {
const simpleMaps = [
['$map: (prop: "flex");', true],
['$font: (italic bold 10px/8pix)', false],
['$map: (prop: /* comment */ 0);', true],
['$calc: calc(100% / 2px);', false],
['$url: url();', false],
];
const nestedMaps = [
['$map: (prop: 0, prop2: (prop3: "normal"), prop4: 2px);', [0, 17]],
['$map: (prop: 0, prop2: (prop3: "normal", prop4: (prop5: "grid")), prop6: 2px);', [0, 17, 42]],
];

test.each(simpleMaps)('simple maps', (css, expected) => {
runTests(css, (decl) => {
const parsedValue = valueParser(decl.value).nodes[0];

expect(isMap(parsedValue)).toBe(expected);
});
});

test.each(nestedMaps)('nested maps', (css, expected) => {
runTests(css, (decl) => {
const parsedValue = valueParser(decl.value);

parsedValue.walk(function(valueNode) {
if (expected.includes(valueNode.sourceIndex)) {
expect(isMap(valueNode)).toBeTruthy();
} else {
expect(isMap(valueNode)).toBeFalsy();
}
});
});
});

it('empty map returns `false`', () => {
const emptyMap = '$map: ();';

runTests(emptyMap, (decl) => {
const parsedValue = valueParser(decl.value);

expect(isMap(parsedValue)).toBeFalsy();
});
});
});

function sassDecls(css, cb) {
sass.parse(css).walkDecls(cb);
}

function scssDecls(css, cb) {
scss.parse(css).walkDecls(cb);
}

function runTests(css, cb) {
[sassDecls, scssDecls].forEach((fn) => {
fn(css, cb);
});
}
45 changes: 45 additions & 0 deletions lib/utils/isMap.js
@@ -0,0 +1,45 @@
/* @flow */
fanich37 marked this conversation as resolved.
Show resolved Hide resolved
'use strict';

/** @typedef {import('postcss-value-parser').Node} ValueNode */

/**
* @param {ValueNode | undefined} valueNode
* @returns {boolean}
*/
module.exports = function(valueNode /*: Object*/) /*: boolean*/ {
fanich37 marked this conversation as resolved.
Show resolved Hide resolved
if (!valueNode) {
return false;
}

if (valueNode.type !== 'function' || !valueNode.nodes || valueNode.value) {
return false;
}

// It's necessary to remove comments and spaces if they are present
const cleanNodes = valueNode.nodes.filter(
(node) => node.type !== 'comment' && node.type !== 'space',
);

// Map without comments and spaces will have the structure like $map (prop: value, prop2: value)
// ↑ ↑ ↑ ↑
// 0 1 2 3
if (cleanNodes[0] && cleanNodes[0].type !== 'word' && cleanNodes[0].type !== 'string') {
return false;
}

if (cleanNodes[1] && cleanNodes[1].value !== ':') {
return false;
}

// There is no need to check type or value of this node since it could be anything
if (!cleanNodes[2]) {
return false;
}

if (cleanNodes[3] && cleanNodes[3].value !== ',') {
return false;
}

return true;
};