Skip to content

Update postcss-less to v3 #6981

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.

Sign up for GitHub

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

Merged
thorn0 merged 32 commits into from
Mar 9, 2020
Merged

Update postcss-less to v3 #6981

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
6fe6109
Update `postcss-less` to v3
fisker Nov 16, 2019
1160388
remove hack
fisker Nov 16, 2019
3f7f2f2
fix inline comment
fisker Nov 16, 2019
86fccab
fix node-important
fisker Nov 16, 2019
c73d9f4
fix less variable
fisker Nov 16, 2019
52fca3b
fix less
fisker Nov 16, 2019
0a9724f
fix ;
fisker Nov 16, 2019
e5a2adc
fix LF after inline comment
fisker Nov 16, 2019
1de072b
update less snapshot
fisker Nov 16, 2019
a9b0a40
fix less
fisker Nov 16, 2019
482946a
spaces
fisker Nov 16, 2019
3a385ec
fix comments
fisker Nov 17, 2019
9f05b86
fix css_case
fisker Nov 17, 2019
d43be21
fix less
fisker Nov 17, 2019
f7d65ea
fix
fisker Nov 17, 2019
0fb8884
clean
fisker Nov 17, 2019
a230e3a
clean
fisker Nov 17, 2019
4c88599
fix nest
fisker Nov 17, 2019
8117419
spaces
fisker Nov 17, 2019
b88d2ed
clean
fisker Nov 17, 2019
c803206
comment
fisker Nov 17, 2019
d4fbcf8
remove includes
fisker Nov 17, 2019
d8cd76c
remove useless delete
fisker Nov 17, 2019
9a6f525
update a LESS case
fisker Nov 17, 2019
1e0424d
fix comment spaces
fisker Nov 17, 2019
0783475
add function
fisker Nov 17, 2019
55e864b
Remove use of '\n'
fisker Jan 27, 2020
ec35e5b
Add parser check
fisker Jan 28, 2020
fc77d17
Add `isLessParser`
fisker Feb 7, 2020
42930a7
Move `lastLineHasInlineComment` to `utils.js`
fisker Feb 7, 2020
717e6dd
Add `12px` & `12PX` test
fisker Feb 7, 2020
73e7ba7
Add `&:EXTEND` test
fisker Feb 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@
"minimatch": "3.0.4",
"minimist": "1.2.0",
"n-readlines": "1.0.0",
"postcss-less": "2.0.0",
"postcss-less": "3.1.4",
"postcss-media-query-parser": "0.2.3",
"postcss-scss": "2.0.0",
"postcss-selector-parser": "2.2.3",
Expand Down
93 changes: 63 additions & 30 deletions src/language-css/parser-postcss.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const createError = require("../common/parser-create-error");
const parseFrontMatter = require("../utils/front-matter");
const lineColumnToIndex = require("../utils/line-column-to-index");
const { hasPragma } = require("./pragma");
const { isSCSS, isSCSSNestedPropertyNode } = require("./utils");
const { isLessParser, isSCSS, isSCSSNestedPropertyNode } = require("./utils");

function parseValueNodes(nodes) {
let parenGroup = {
Expand Down Expand Up @@ -199,10 +199,10 @@ function parseSelector(selector) {
// the content of the comment as selectors which turns it into complete
// garbage. Better to print the whole selector as-is and not try to parse
// and reformat it.
if (selector.match(/\/\/|\/\*/)) {
if (/\/\/|\/\*/.test(selector)) {
return {
type: "selector-unknown",
value: selector.replace(/^ +/, "").replace(/ +$/, "")
value: selector.trim()
};
}

Expand Down Expand Up @@ -283,16 +283,6 @@ function parseNestedCSS(node, options) {
node.raws.selector = selector;
}

// [email protected] parse `custom-selector` as `css-decl`
if (
options.parser === "css" &&
node.type === "css-decl" &&
node.prop === "@custom-selector"
) {
selector = node.value;
node.raws.value = selector;
}

let value = "";

if (typeof node.value === "string") {
Expand Down Expand Up @@ -385,6 +375,64 @@ function parseNestedCSS(node, options) {
node.value = parseValue(value);
}

if (node.type === "css-atrule") {
if (isLessParser(options)) {
// mixin
if (node.mixin) {
const source =
node.raws.identifier +
node.name +
node.raws.afterName +
node.raws.params;
node.selector = parseSelector(source);
delete node.params;
return node;
}

// function
if (node.function) {
return node;
}
}

// only css support custom-selector
if (options.parser === "css" && node.name === "custom-selector") {
const customSelector = node.params.match(/:--\S+?\s+/)[0].trim();
Comment thread
fisker marked this conversation as resolved.
node.customSelector = customSelector;
node.selector = parseSelector(
node.params.slice(customSelector.length).trim()
);
delete node.params;
return node;
}

if (isLessParser(options)) {
// Whitespace between variable and colon
if (node.name.includes(":") && !node.params) {
node.variable = true;
const parts = node.name.split(":");
node.name = parts[0];
node.value = parseValue(parts.slice(1).join(":"));
}

// Missing whitespace between variable and colon
if (
!["page", "nest"].includes(node.name) &&
node.params &&
node.params[0] === ":"
) {
node.variable = true;
node.value = parseValue(node.params.slice(1));
}

// Less variable
if (node.variable) {
delete node.params;
return node;
}
}
}

if (node.type === "css-atrule" && params.length > 0) {
const { name } = node;
const lowercasedName = node.name.toLowerCase();
Expand Down Expand Up @@ -417,6 +465,8 @@ function parseNestedCSS(node, options) {
}

if (lowercasedName === "import") {
node.import = true;
delete node.filename;
Comment thread
fisker marked this conversation as resolved.
node.params = parseValue(params);
return node;
}
Expand Down Expand Up @@ -450,16 +500,6 @@ function parseNestedCSS(node, options) {
return node;
}

if (name === "custom-selector") {
const customSelector = params.match(/:--\S+?\s+/)[0].trim();

node.customSelector = customSelector;
node.selector = parseSelector(params.slice(customSelector.length));
delete node.params;

return node;
}

if (["media", "custom-media"].includes(lowercasedName)) {
if (params.includes("#{")) {
// Workaround for media at rule with scss interpolation
Expand Down Expand Up @@ -513,13 +553,6 @@ function requireParser(isSCSSParser) {
return require("postcss-scss");
}

// TODO: Remove this hack when this issue is fixed:
// https://github.com/shellscape/postcss-less/issues/88
const LessParser = require("postcss-less/dist/less-parser");
LessParser.prototype.atrule = function(...args) {
return Object.getPrototypeOf(LessParser.prototype).atrule.apply(this, args);
};

return require("postcss-less");
}

Expand Down
73 changes: 66 additions & 7 deletions src/language-css/printer-postcss.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ const {
isWideKeywords,
isSCSS,
isLastNode,
isLessParser,
isSCSSControlDirectiveNode,
isDetachedRulesetDeclarationNode,
isRelationalOperatorNode,
Expand Down Expand Up @@ -68,7 +69,8 @@ const {
isWordNode,
isColonNode,
isMediaAndSupportsKeywords,
isColorAdjusterFuncNode
isColorAdjusterFuncNode,
lastLineHasInlineComment
} = require("./utils");

function shouldPrintComma(options) {
Expand Down Expand Up @@ -115,12 +117,20 @@ function genericPrint(path, options, print) {
options.locStart(node),
options.locEnd(node)
);

const rawText = node.raws.text || node.text;
// Workaround a bug where the location is off.
// https://github.com/postcss/postcss-scss/issues/63
if (!text.includes(rawText)) {
if (node.raws.inline) {
return concat(["// ", rawText]);
if (node.raws.inline || node.inline) {
const needBreakAfter = !(
node.source.input.css.split(/[\r\n]/)[node.source.end.line] || ""
).trim();
return concat([
"//",
node.raws.left + rawText,
needBreakAfter ? line : ""
]);
Copy link
Copy Markdown
Member

@alexander-akait alexander-akait Jan 28, 2020

Choose a reason for hiding this comment

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

I think we can simplify that, it was workaround for less parsers bugs, so maybe they were fixed

Copy link
Copy Markdown
Member Author

@fisker fisker Jan 28, 2020

Choose a reason for hiding this comment

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

Will check this

Copy link
Copy Markdown
Member Author

@fisker fisker Jan 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

#7021 seems fixing this?

Copy link
Copy Markdown
Member

@alexander-akait alexander-akait Feb 7, 2020

Choose a reason for hiding this comment

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

Yes, let's keep that as is here, and refactor in other PRs

}
return concat(["/* ", rawText, " */"]);
}
Expand All @@ -132,7 +142,12 @@ function genericPrint(path, options, print) {
node.important ? " !important" : "",
node.nodes
? concat([
" {",
node.selector &&
node.selector.type === "selector-unknown" &&
lastLineHasInlineComment(node.selector.value)
Copy link
Copy Markdown
Member

@alexander-akait alexander-akait Feb 7, 2020

Choose a reason for hiding this comment

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

👍

? line
: " ",
"{",
node.nodes.length > 0
? indent(
concat([hardline, printNodeSequence(path, options, print)])
Expand Down Expand Up @@ -189,6 +204,52 @@ function genericPrint(path, options, print) {
}
case "css-atrule": {
const parentNode = path.getParentNode();
const isTemplatePlaceholderNodeWithoutSemiColon =
isTemplatePlaceholderNode(node) &&
!parentNode.raws.semicolon &&
options.originalText[options.locEnd(node) - 1] !== ";";

if (isLessParser(options)) {
if (node.mixin) {
return concat([
path.call(print, "selector"),
node.important ? " !important" : "",
isTemplatePlaceholderNodeWithoutSemiColon ? "" : ";"
]);
}

if (node.function) {
return concat([
node.name,
concat([path.call(print, "params")]),
isTemplatePlaceholderNodeWithoutSemiColon ? "" : ";"
]);
}

if (node.variable) {
return concat([
"@",
node.name,
": ",
node.value ? concat([path.call(print, "value")]) : "",
node.raws.between.trim() ? node.raws.between.trim() + " " : "",
node.nodes
? concat([
"{",
indent(
concat([
node.nodes.length > 0 ? softline : "",
printNodeSequence(path, options, print)
])
),
softline,
"}"
])
: "",
isTemplatePlaceholderNodeWithoutSemiColon ? "" : ";"
]);
}
}
Copy link
Copy Markdown
Member

@alexander-akait alexander-akait Nov 18, 2019

Choose a reason for hiding this comment

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

Each node.mixin/node.function/node.variable should be:

if (isLessParser) { 
  if (node.mixin) { /* ... */ }
  if (node.function) { /* ... */ }
}

It is better for performance and other parsers can potentially have same properties

Copy link
Copy Markdown
Member Author

@fisker fisker Nov 18, 2019

Choose a reason for hiding this comment

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

will do

Copy link
Copy Markdown
Member

@alexander-akait alexander-akait Jan 28, 2020

Choose a reason for hiding this comment

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

Please add isLessParser check here

Copy link
Copy Markdown
Member Author

@fisker fisker Jan 28, 2020

Choose a reason for hiding this comment

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

In fact we can't use options.parser === "less", since we has parser=css also use less parser, so I use options.parser !== "scss" in this commit

07010de

What do you think? or maybe options.parser === "less" || options.parser === "csss"?

Copy link
Copy Markdown
Member

@alexander-akait alexander-akait Jan 28, 2020

Choose a reason for hiding this comment

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

To be honestly we should throw error on less syntax when developer set css parser. What about change the options.parser value change when we switch on other syntax?

Copy link
Copy Markdown
Member Author

@fisker fisker Jan 28, 2020

Choose a reason for hiding this comment

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

Maybe we can do this later in a seperate PR, shall we?

Copy link
Copy Markdown
Member

@alexander-akait alexander-akait Jan 28, 2020

Choose a reason for hiding this comment

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

Yes, don't forget open new issue after merge, anyway let's encapsulate logic in one util function isLessParser, it is allow to fix problem in one places


return concat([
"@",
Expand Down Expand Up @@ -242,9 +303,7 @@ function genericPrint(path, options, print) {
softline,
"}"
])
: isTemplatePlaceholderNode(node) &&
!parentNode.raws.semicolon &&
options.originalText[options.locEnd(node) - 1] !== ";"
: isTemplatePlaceholderNodeWithoutSemiColon
? ""
: ";"
]);
Expand Down
13 changes: 12 additions & 1 deletion src/language-css/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,15 @@ function isColorAdjusterFuncNode(node) {
return colorAdjusterFunctions.includes(node.value.toLowerCase());
}

// TODO: only check `less` when we don't use `less` to parse `css`
function isLessParser(options) {
return options.parser === "css" || options.parser === "less";
}

function lastLineHasInlineComment(text) {
return /\/\//.test(text.split(/[\r\n]/).pop());
}

module.exports = {
getAncestorCounter,
getAncestorNode,
Expand All @@ -392,6 +401,7 @@ module.exports = {
isWideKeywords,
isSCSS,
isLastNode,
isLessParser,
isSCSSControlDirectiveNode,
isDetachedRulesetDeclarationNode,
isRelationalOperatorNode,
Expand Down Expand Up @@ -424,5 +434,6 @@ module.exports = {
isWordNode,
isColonNode,
isMediaAndSupportsKeywords,
isColorAdjusterFuncNode
isColorAdjusterFuncNode,
lastLineHasInlineComment
};
12 changes: 9 additions & 3 deletions tests/css_case/__snapshots__/jsfmt.spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,13 +217,16 @@ $KeepScssVar: val;

@KeepDetachedRuleset();

&:extend(.Keep ALL);
Copy link
Copy Markdown
Member Author

@fisker fisker Nov 17, 2019

Choose a reason for hiding this comment

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

#6981 (comment)

Copy link
Copy Markdown
Member

@alexander-akait alexander-akait Feb 7, 2020

Choose a reason for hiding this comment

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

we should keep EXTEND as is

.bucket {
  tr { // nested ruleset with target selector
    color: blue;
  }
}
.some-class:extend(.bucket tr) {}

->

.bucket tr,
.some-class {
  color: blue;
}

But:

.bucket {
  tr { // nested ruleset with target selector
    color: blue;
  }
}
.some-class:EXTEND(.bucket tr) {}

->

.bucket tr {
  color: blue;
}

&:EXTEND(.Keep ALL);

.Keep;
.Keep();
.Keep(4PX)!IMPORTANT;
.Keep() when (@Keep=Keep);
.Keep() when (@Keep=12PX);
.Keep() when (@Keep="12px");
.Keep() when (@Keep="12PX");
.Keep() when (@Keep=Keep12PX);
}

Expand Down Expand Up @@ -308,7 +311,7 @@ svg[viewBox] linearGradient,
--Keep-custom-Prop: red;
background: Var(--Keep-custom-Prop);
animation-name: KeepAnimationName;
important: something !IMPORTANT;
important: something !important;
font-family: initial;
padding: unset;
border: inherit;
Expand Down Expand Up @@ -364,13 +367,16 @@ $KeepScssVar: val;

@KeepDetachedRuleset();

&: EXTEND(.Keep ALL);
&:extend(.Keep ALL);
Copy link
Copy Markdown
Member Author

@fisker fisker Nov 17, 2019

Choose a reason for hiding this comment

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

#6981 (comment)

&:EXTEND(.Keep ALL);
Copy link
Copy Markdown
Member Author

@fisker fisker Nov 17, 2019

Choose a reason for hiding this comment

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

Anyone know what is this? space matters?

Copy link
Copy Markdown
Member

@lydell lydell Nov 17, 2019

Choose a reason for hiding this comment

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

I think I added this test case when I added support for lowercasing of stuff back in the day. Testing on http://lesscss.org/less-preview/, uppercase EXTEND does not appear to be valid Less. So it’s a weird test case. &:extend(.Keep ALL) seems to be valid syntax, while &: extend(.Keep ALL) is a syntax error.

Copy link
Copy Markdown
Member Author

@fisker fisker Nov 17, 2019

Choose a reason for hiding this comment

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

what output is expect?

Copy link
Copy Markdown
Member

@lydell lydell Nov 17, 2019

Choose a reason for hiding this comment

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

My guess is that the input should be changed to &:extend(.Keep ALL) and the output should be the same (&:extend(.Keep ALL)).


.Keep;
.Keep();
.Keep(4px) !important;
.Keep() when (@Keep=Keep);
.Keep() when (@Keep=12PX);
.Keep() when (@Keep=12px);
Copy link
Copy Markdown
Member

@alexander-akait alexander-akait Nov 19, 2019

Choose a reason for hiding this comment

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

We should keep 12PX

Copy link
Copy Markdown
Member

@alexander-akait alexander-akait Feb 7, 2020

Choose a reason for hiding this comment

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

Can we add test for .Keep() when (@Keep="12px");?

.Keep() when (@Keep="12px");
.Keep() when (@Keep="12PX");
.Keep() when (@Keep=Keep12PX);
}

Expand Down
3 changes: 3 additions & 0 deletions tests/css_case/case.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,16 @@ $KeepScssVar: val;

@KeepDetachedRuleset();

&:extend(.Keep ALL);
&:EXTEND(.Keep ALL);

.Keep;
.Keep();
.Keep(4PX)!IMPORTANT;
.Keep() when (@Keep=Keep);
.Keep() when (@Keep=12PX);
.Keep() when (@Keep="12px");
.Keep() when (@Keep="12PX");
.Keep() when (@Keep=Keep12PX);
}

Expand Down
Loading