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

Update postcss to v8 #9209

Merged
merged 25 commits into from
Sep 24, 2020
Merged
Show file tree
Hide file tree
Changes from all 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
42 changes: 42 additions & 0 deletions changelog_unreleased/css/pr-9209.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
#### Improve custom properties format (#9209 by @fisker)

Thanks to [`PostCSS 8.0`](https://github.com/postcss/postcss/releases/tag/8.0.0), we can handle these edge cases on custom properties.

<!-- prettier-ignore -->
```css
/* Input */
:root {
--empty: ;
--JSON: [1, "2", {"three": {"a":1}}, [4]];
--javascript: function(rule) { console.log(rule) };
}

@supports (--element(".minwidth", { "minWidth": 300 })) {
[--self] {
background: greenyellow;
}
}

/* Prettier stable */
SyntaxError: (postcss) CssSyntaxError Missed semicolon (3:20)
1 | :root {
2 | --empty: ;
> 3 | --JSON: [1, "2", {"three": {"a":1}}, [4]];
| ^
4 | --javascript: function(rule) { console.log(rule) };
5 | }
6 |

/* Prettier master */
:root {
--empty: ;
--JSON: [1, "2", {"three": {"a": 1}}, [4]];
--javascript: function(rule) {console.log(rule)};
}

@supports (--element(".minwidth", {"minWidth": 300})) {
[--self] {
background: greenyellow;
}
}
```
11 changes: 9 additions & 2 deletions jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,15 @@ if (isProduction) {
presets: [
[
"@babel/env",
// Workaround for https://github.com/babel/babel/issues/11994
{ loose: true },
{
targets: { node: "current" },
exclude: [
"transform-async-to-generator",
"transform-classes",
"proposal-async-generator-functions",
"transform-regenerator",
],
},
],
],
},
Expand Down
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@
"outdent": "0.7.1",
"parse-srcset": "ikatyang/parse-srcset#54eb9c1cb21db5c62b4d0e275d7249516df6f0ee",
"please-upgrade-node": "3.2.0",
"postcss": "7.0.32",
"postcss": "8.0.7",
"postcss-less": "3.1.4",
"postcss-media-query-parser": "0.2.3",
"postcss-scss": "2.1.1",
Expand Down Expand Up @@ -145,7 +145,7 @@
"lint:eslint": "cross-env EFF_NO_LINK_RULES=true eslint . --format friendly",
"lint:changelog": "node ./scripts/lint-changelog.js",
"lint:prettier": "prettier . \"!test*\" --check",
"lint:dist": "eslint --no-eslintrc --no-ignore --env=es6,browser --parser-options=ecmaVersion:2016 \"dist/!(bin-prettier|index|third-party).js\"",
"lint:dist": "eslint --no-eslintrc --no-ignore --env=es6,browser --parser-options=ecmaVersion:2017 \"dist/!(bin-prettier|index|third-party).js\"",
"lint:spellcheck": "cspell \"**/*\" \".github/**/*\"",
"lint:deps": "node ./scripts/check-deps.js",
"fix": "run-s fix:eslint fix:prettier",
Expand Down
52 changes: 52 additions & 0 deletions src/language-css/parser-postcss.js
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,57 @@ function parseNestedCSS(node, options) {
node.raws = {};
}

// Custom properties looks like declarations
if (
options.parser === "css" &&
node.type === "css-decl" &&
typeof node.prop === "string" &&
node.prop.startsWith("--") &&
typeof node.value === "string" &&
node.value.startsWith("{")
) {
let rules;
if (node.value.endsWith("}")) {
const textBefore = options.originalText.slice(
0,
node.source.start.offset
);
const nodeText =
"a".repeat(node.prop.length) +
options.originalText.slice(
node.source.start.offset + node.prop.length,
node.source.end.offset + 1
);
const fakeContent = textBefore.replace(/[^\n]/g, " ") + nodeText;
let ast;
try {
ast = parseCss(fakeContent, [], { ...options });
} catch (_) {
// noop
}
if (
ast &&
ast.nodes &&
ast.nodes.length === 1 &&
ast.nodes[0].type === "css-rule"
) {
rules = ast.nodes[0].nodes;
}
}
if (rules) {
node.value = {
type: "css-rule",
nodes: rules,
};
} else {
node.value = {
type: "value-unknown",
value: node.raws.value.raw,
};
}
return node;
}
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 we should remove them in the future major release, it is invalid

Copy link
Member Author

Choose a reason for hiding this comment

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

I was going to parse is as value-unknown, but seems it is a proposal, but abandoned

http://tabatkins.github.io/specs/css-apply-rule/#defining

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but it is violate CSS spec, so prefer to avoid this, because it can break some future CSS features


let selector = "";

if (typeof node.selector === "string") {
Expand Down Expand Up @@ -594,6 +645,7 @@ function parseWithParser(parse, text, options) {
throw createError("(postcss) " + e.name + " " + e.reason, { start: e });
}

options.originalText = text;
result = parseNestedCSS(addTypePrefix(result, "css-"), options);

calculateLoc(result, text);
Expand Down
8 changes: 6 additions & 2 deletions src/language-css/printer-postcss.js
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,9 @@ function genericPrint(path, options, print) {
node.selector.type === "selector-unknown" &&
lastLineHasInlineComment(node.selector.value)
? line
: " ",
: node.selector
? " "
: "",
"{",
node.nodes.length > 0
? indent(
Expand All @@ -141,6 +143,8 @@ function genericPrint(path, options, print) {
const { between: rawBetween } = node.raws;
const trimmedBetween = rawBetween.trim();
const isColon = trimmedBetween === ":";
const isValueAllSpace =
typeof node.value === "string" && /^ *$/.test(node.value);

let value = hasComposesNode(node)
? removeLines(path.call(print, "value"))
Expand All @@ -155,7 +159,7 @@ function genericPrint(path, options, print) {
insideICSSRuleNode(path) ? node.prop : maybeToLowerCase(node.prop),
trimmedBetween.startsWith("//") ? " " : "",
trimmedBetween,
node.extend ? "" : " ",
node.extend || isValueAllSpace ? "" : " ",
options.parser === "less" && node.extend && node.selector
? concat(["extend(", path.call(print, "selector"), ")"])
: "",
Expand Down
42 changes: 40 additions & 2 deletions tests/css/comments/__snapshots__/jsfmt.spec.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,44 @@ printWidth: 80
================================================================================
`;

exports[`custom-properties.css format 1`] = `
====================================options=====================================
parsers: ["css"]
printWidth: 80
| printWidth
=====================================input======================================
/* comment 1 */
:root {
/* comment 2 */
--prop : {
/* comment 3 */
color/* comment 4 */: /* comment 5 */#fff/* comment 6 */;/* comment 7 */
/* comment 8 */
font-size: 12px;
/* comment 9 */
};
/* comment 10 */
}
/* comment 11 */

=====================================output=====================================
/* comment 1 */
:root {
/* comment 2 */
--prop: {
/* comment 3 */
color/* comment 4 */: /* comment 5 */ #fff /* comment 6 */; /* comment 7 */
/* comment 8 */
font-size: 12px;
/* comment 9 */
};
/* comment 10 */
}
/* comment 11 */

================================================================================
`;

exports[`declaration.css format 1`] = `
====================================options=====================================
parsers: ["css"]
Expand Down Expand Up @@ -887,11 +925,11 @@ article /* comment 168 */ :--heading /* comment 169 */ + /* comment 170 */ p /*
/* custom properties set & @apply rule */
:root {
/* comments 192 */
--centered /* comments 193 */ : /* comments 194 */ {
--centered/* comments 193 */ : /* comments 194 */ {
display: flex;
align-items: center;
justify-content: center;
}
};
Copy link
Member

Choose a reason for hiding this comment

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

Regression?

Copy link
Member Author

@fisker fisker Sep 16, 2020

Choose a reason for hiding this comment

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

Acutally, I think it's bugfix.

--centered is a prop, it should add ;

The space before /* comments 193 */ is gone, it's because we print decl like this, playground.

There is a space before, because we treat it as rule's selector before, playground.

Copy link
Member Author

Choose a reason for hiding this comment

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

And the test is added by you, the input has ;

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should print less and scss like this too.

Copy link
Member

Choose a reason for hiding this comment

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

scss (need test on less) supports nested declarations https://www.sassmeister.com/, so prefer keep it as is

Copy link
Member Author

Choose a reason for hiding this comment

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

The whole part after --foo is parsed as value. And sent to value-parser, how do we tell this should not parse as value?

Copy link
Member Author

Choose a reason for hiding this comment

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

Starts with {?

Copy link
Member

Choose a reason for hiding this comment

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

It we have -- at the start and : {

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I'll change this case to let the value parse as value-unknown

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, c5e3bdd.

}

================================================================================
Expand Down
13 changes: 13 additions & 0 deletions tests/css/comments/custom-properties.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/* comment 1 */
:root {
/* comment 2 */
--prop : {
/* comment 3 */
color/* comment 4 */: /* comment 5 */#fff/* comment 6 */;/* comment 7 */
/* comment 8 */
font-size: 12px;
/* comment 9 */
};
/* comment 10 */
}
/* comment 11 */
71 changes: 71 additions & 0 deletions tests/css/postcss-8-improment/__snapshots__/jsfmt.spec.js.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`empty-props.css format 1`] = `
====================================options=====================================
parsers: ["css"]
printWidth: 80
| printWidth
=====================================input======================================
:root {
--empty:;
--one-space: ;
--two-space: ;
--many-space: ;
}

=====================================output=====================================
:root {
--empty:;
--one-space: ;
--two-space: ;
--many-space: ;
}

================================================================================
`;

exports[`test.css format 1`] = `
====================================options=====================================
parsers: ["css"]
printWidth: 80
| printWidth
=====================================input======================================
/*
This test is copied from \`postcss@8\` release note

https://github.com/postcss/postcss/releases/tag/8.0.0
*/

:root {
--empty: ;
--JSON: [1, "2", {"three": {"a":1}}, [4]];
--javascript: function(rule) { console.log(rule) };
}

@supports (--element(".minwidth", { "minWidth": 300 })) {
[--self] {
background: greenyellow;
}
}

=====================================output=====================================
/*
This test is copied from \`postcss@8\` release note

https://github.com/postcss/postcss/releases/tag/8.0.0
*/

:root {
--empty: ;
Copy link
Member

Choose a reason for hiding this comment

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

Good catch 👍

Copy link
Member

Choose a reason for hiding this comment

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

--empty:; is invalid syntax
--empty: ; is valid syntax

So both should not be changed

--JSON: [1, "2", {"three": {"a": 1}}, [4]];
--javascript: function(rule) {console.log(rule)};
}

@supports (--element(".minwidth", {"minWidth": 300})) {
[--self] {
background: greenyellow;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we add fixes to the PR message and add changelog? Or you can do it in the separate PR?


================================================================================
`;
6 changes: 6 additions & 0 deletions tests/css/postcss-8-improment/empty-props.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
:root {
--empty:;
--one-space: ;
--two-space: ;
--many-space: ;
}
1 change: 1 addition & 0 deletions tests/css/postcss-8-improment/jsfmt.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
run_spec(__dirname, ["css"]);
17 changes: 17 additions & 0 deletions tests/css/postcss-8-improment/test.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/*
This test is copied from `postcss@8` release note

https://github.com/postcss/postcss/releases/tag/8.0.0
*/

:root {
--empty: ;
--JSON: [1, "2", {"three": {"a":1}}, [4]];
--javascript: function(rule) { console.log(rule) };
}

@supports (--element(".minwidth", { "minWidth": 300 })) {
[--self] {
background: greenyellow;
}
}