Skip to content

Commit

Permalink
Fix: Prettier breaks code when inline number lists are used as arbitr…
Browse files Browse the repository at this point in the history 
…ary arguments [SCSS] (#8566)
  • Loading branch information
@boyenn
boyenn committed Jun 18, 2020
1 parent dc185c8 commit 4009ecf
Show file tree
Hide file tree
Showing 4 changed files with 86 additions and 17 deletions.
14 changes: 10 additions & 4 deletions changelog_unreleased/css/pr-8567.md
View file
@@ -1,22 +1,28 @@
#### Fix: [Sass] Unexpectedly add extra space after function when passing arbitrary arguments ([#8567](https://github.com/prettier/prettier/pull/8567) by [@boyenn](https://github.com/boyenn))
#### Improve Prettier handling of arbitrary arguments ([#8567](https://github.com/prettier/prettier/pull/8567), [#8566](https://github.com/prettier/prettier/pull/8566) by [@boyenn](https://github.com/boyenn))

Prettier no longer unexpectedly add extra space after function when passing arbitrary arguments.
Prettier no longer breaks code when inline number lists are used as arbitrary arguments.

<!-- prettier-ignore -->
```css
/* Input */
body {
test: function($list...);
test: function(return-list($list)...);
foo: bar(returns-list($list)...);
background-color: rgba(50 50 50 50...);
}

/* Prettier stable */
body {
test: function($list...);
test: function(return-list($list) ...);
foo: bar(returns-list($list) ...);
background-color: rgba(50 50 50 50..);
}

/* Prettier master */
body {
test: function($list...);
test: function(return-list($list)...);
foo: bar(returns-list($list)...);
background-color: rgba(50 50 50 50...);
}
```
39 changes: 26 additions & 13 deletions src/language-css/parser-postcss.js
View file
Expand Up @@ -14,7 +14,7 @@ const {
} = require("./utils");
const { calculateLoc, replaceQuotesInInlineComments } = require("./loc");

function parseValueNodes(nodes) {
function parseValueNodes(nodes, options) {
let parenGroup = {
open: null,
close: null,
Expand All @@ -32,6 +32,19 @@ function parseValueNodes(nodes) {
for (let i = 0; i < nodes.length; ++i) {
const node = nodes[i];

if (
isSCSS(options.parser, node.value) &&
node.type === "number" &&
node.unit === ".." &&
node.value[node.value.length - 1] === "."
) {
// Work around postcss bug parsing `50...` as `50.` with unit `..`
// Set the unit to `...` to "accidentally" have arbitrary arguments work in the same way that cases where the node already had a unit work.
// For example, 50px... is parsed as `50` with unit `px...` already by postcss-values-parser.
node.value = node.value.slice(0, -1);
node.unit = "...";
}

if (node.type === "func" && node.value === "url") {
const groups = (node.group && node.group.groups) || [];

Expand Down Expand Up @@ -153,21 +166,21 @@ function addMissingType(node) {
return node;
}

function parseNestedValue(node) {
function parseNestedValue(node, options) {
if (node && typeof node === "object") {
delete node.parent;
for (const key in node) {
parseNestedValue(node[key]);
parseNestedValue(node[key], options);
if (key === "nodes") {
node.group = flattenGroups(parseValueNodes(node[key]));
node.group = flattenGroups(parseValueNodes(node[key], options));
delete node[key];
}
}
}
return node;
}

function parseValue(value) {
function parseValue(value, options) {
const valueParser = require("postcss-values-parser");

let result = null;
Expand All @@ -183,7 +196,7 @@ function parseValue(value) {

result.text = value;

const parsedResult = parseNestedValue(result);
const parsedResult = parseNestedValue(result, options);

return addTypePrefix(parsedResult, "value-");
}
Expand Down Expand Up @@ -321,7 +334,7 @@ function parseNestedCSS(node, options) {

// Ignore LESS mixins
if (node.mixin) {
node.selector = parseValue(selector);
node.selector = parseValue(selector, options);

return node;
}
Expand Down Expand Up @@ -366,7 +379,7 @@ function parseNestedCSS(node, options) {
};
}

node.value = parseValue(value);
node.value = parseValue(value, options);
}

if (
Expand Down Expand Up @@ -423,7 +436,7 @@ function parseNestedCSS(node, options) {
node.variable = true;
const parts = node.name.split(":");
node.name = parts[0];
node.value = parseValue(parts.slice(1).join(":"));
node.value = parseValue(parts.slice(1).join(":"), options);
}

// Missing whitespace between variable and colon
Expand All @@ -433,7 +446,7 @@ function parseNestedCSS(node, options) {
node.params[0] === ":"
) {
node.variable = true;
node.value = parseValue(node.params.slice(1));
node.value = parseValue(node.params.slice(1), options);
}

// Less variable
Expand Down Expand Up @@ -466,7 +479,7 @@ function parseNestedCSS(node, options) {

if (name === "at-root") {
if (/^\(\s*(without|with)\s*:[\S\s]+\)$/.test(params)) {
node.params = parseValue(params);
node.params = parseValue(params, options);
} else {
node.selector = parseSelector(params);
delete node.params;
Expand All @@ -478,7 +491,7 @@ function parseNestedCSS(node, options) {
if (lowercasedName === "import") {
node.import = true;
delete node.filename;
node.params = parseValue(params);
node.params = parseValue(params, options);
return node;
}

Expand All @@ -505,7 +518,7 @@ function parseNestedCSS(node, options) {
// Remove unnecessary spaces before SCSS control, mixin and function directives
params = params.replace(/^(?!if)(\S+)\s+\(/, "$1(");

node.value = parseValue(params);
node.value = parseValue(params, options);
delete node.params;

return node;
Expand Down
40 changes: 40 additions & 0 deletions tests/scss/scss/__snapshots__/jsfmt.spec.js.snap
View file
Expand Up @@ -39,6 +39,16 @@ $parameters: (
);
$value: dummy($parameters...);
body {
background-color: rgba(50, 50, 50, 50);
background-color: rgba(50 50 50 50...);
background-color: rgba(50 50 .50 50...);
background-color: rgba(50 50 50. .50...);
// Input is not technically valid ( output is ), but still nice to know that the \`.\` gets dropped as it would for \`50.\`
background-color: rgba(50 50 50 50....);
width: min(50px 20px 30px...);
}
=====================================output=====================================
body {
test: foo(return-list($list)...);
Expand Down Expand Up @@ -72,6 +82,16 @@ $parameters: (
);
$value: dummy($parameters...);
body {
background-color: rgba(50, 50, 50, 50);
background-color: rgba(50 50 50 50...);
background-color: rgba(50 50 0.5 50...);
background-color: rgba(50 50 50 0.5...);
// Input is not technically valid ( output is ), but still nice to know that the \`.\` gets dropped as it would for \`50.\`
background-color: rgba(50 50 50 50...);
width: min(50px 20px 30px...);
}
================================================================================
`;

Expand Down Expand Up @@ -113,6 +133,16 @@ $parameters: (
);
$value: dummy($parameters...);
body {
background-color: rgba(50, 50, 50, 50);
background-color: rgba(50 50 50 50...);
background-color: rgba(50 50 .50 50...);
background-color: rgba(50 50 50. .50...);
// Input is not technically valid ( output is ), but still nice to know that the \`.\` gets dropped as it would for \`50.\`
background-color: rgba(50 50 50 50....);
width: min(50px 20px 30px...);
}
=====================================output=====================================
body {
test: foo(return-list($list)...);
Expand Down Expand Up @@ -146,6 +176,16 @@ $parameters: (
);
$value: dummy($parameters...);
body {
background-color: rgba(50, 50, 50, 50);
background-color: rgba(50 50 50 50...);
background-color: rgba(50 50 0.5 50...);
background-color: rgba(50 50 50 0.5...);
// Input is not technically valid ( output is ), but still nice to know that the \`.\` gets dropped as it would for \`50.\`
background-color: rgba(50 50 50 50...);
width: min(50px 20px 30px...);
}
================================================================================
`;

Expand Down
10 changes: 10 additions & 0 deletions tests/scss/scss/arbitrary-arguments.scss
View file
Expand Up @@ -29,3 +29,13 @@ $parameters: (
'b': 42
);
$value: dummy($parameters...);

body {
background-color: rgba(50, 50, 50, 50);
background-color: rgba(50 50 50 50...);
background-color: rgba(50 50 .50 50...);
background-color: rgba(50 50 50. .50...);
// Input is not technically valid ( output is ), but still nice to know that the `.` gets dropped as it would for `50.`
background-color: rgba(50 50 50 50....);
width: min(50px 20px 30px...);
}

0 comments on commit 4009ecf

Please sign in to comment.