Update `postcss-less` to v3 by fisker · Pull Request #6981 · prettier/prettier

fisker · 2019-11-17T06:26:47Z

I’ve added tests to confirm my change works.
(If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
(If the change is user-facing) I’ve added my changes to changelog_unreleased/*/pr-XXXX.md file following changelog_unreleased/TEMPLATE.md.
I’ve read the contributing guidelines.

fisker · 2019-11-17T06:32:11Z

  @KeepDetachedRuleset();

-  &: EXTEND(.Keep ALL);
+  &:EXTEND(.Keep ALL);


Anyone know what is this? space matters?

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.

what output is expect?

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

lydell · 2019-11-17T10:01:45Z

Also feel free to switch to the next branch if Node.js 4 is giving you trouble – we should not waste more time on that old version.

fisker · 2019-11-17T13:28:11Z

    @KeepDetachedRuleset();

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


#6981 (comment)

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; }

fisker · 2019-11-17T13:28:16Z

  @KeepDetachedRuleset();

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


#6981 (comment)

fisker · 2019-11-17T13:48:52Z

ready for review

thorn0 · 2019-11-17T16:40:38Z

each is still not handled correctly, but at least it's not a parser error as it was before (#5653).

@ should not be added in front of each
whitespace should not be added in front of @{value}
anonymous ruleset should not be formatted inline

Prettier pr-6981
Playground link

--parser less

Input:

@selectors: blue, green, red;

each(@selectors, {
  .sel-@{value} {
    a: b;
  }
});

Output:

@selectors: blue, green, red;

@each (@selectors, {.sel- @{value} {a: b;}}) ;

fisker · 2019-11-17T16:48:58Z

looking into this, different at-rule with function: true

fisker · 2019-11-17T17:27:35Z

@thorn0 add a test for it, print as it is, maybe we can make it pretty later, I guess ugly is better than broken. this PR only update this module

fisker · 2019-11-17T17:34:33Z

Prettier pr-6981
Playground link

--parser less

Input:

@selectors: blue, green, red;

each(@selectors, {
  .sel-@{value} {
    
a: b;
  }
});

Output:

@selectors: blue, green, red;

each(@selectors, {
  .sel-@{value} {
    
a: b;
  }
});

thorn0 · 2019-11-17T17:35:29Z

@fisker 👍 I'm okay with whatever you decide as long as the output stays valid. First correctness, then aesthetics.

thorn0 · 2019-11-17T20:50:18Z

#5420 is not a parser error now, but selectors assigned to a variable (docs) get corrupted by whitespace inserted after colons.

Prettier pr-6981
Playground link

--parser less

Input:

@selector: &:not([type='file']):not([type='checkbox']):not([type='radio']):not([type='range']):not([type='hidden']);

Output:

@selector: &: not([type= "file" ]): not([type= "checkbox" ]):
  not([type= "radio" ]): not([type= "range" ]): not([type= "hidden" ]);

fisker · 2019-11-18T08:06:05Z

variable support selector, maybe we need do a isSelector check, but should do in a separate PR

alexander-akait

Many problems, we should fix it

alexander-akait · 2019-11-18T10:39:00Z

+            : "",
+          isTemplatePlaceholderNodeWithoutSemiColon ? "" : ";"
+        ]);
+      }


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

Please add isLessParser check here

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"?

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?

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

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

alexander-akait · 2019-11-18T10:39:32Z

-              " {",
+              node.selector &&
+              node.selector.type === "selector-unknown" &&
+              node.selector.value.endsWith("\n")


Is is bad idea use node.selector.value.endsWith("\n")

alexander-akait · 2019-11-18T10:41:13Z

+        if (node.raws.inline || node.inline) {
+          const needBreakAfter = !(
+            node.source.input.css.split("\n")[node.source.end.line] || ""
+          ).trim();
+          return concat([
+            "//",
+            node.raws.left + rawText,
+            needBreakAfter ? "\n" : ""
+          ]);


"\n"? We should use prettier builders
node.source.input.css should be never used

this place is very bad, need rewrite this

alexander-akait · 2019-11-18T10:43:02Z

+    // last line has inline comment
+    if (/\/\//.test(selector.split("\n").pop())) {
+      value += "\n";
+    }


Why? Using \n break code for developers who using CRLF

alexander-akait · 2019-11-18T10:43:51Z

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


variable, use value property

#6981 (comment)

alexander-akait · 2019-11-18T10:44:20Z

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


alexander-akait · 2019-11-18T10:45:23Z


+    if (node.type === "css-atrule") {
+      // mixin
+      if (node.mixin) {


isLessParser && node.mixin

alexander-akait · 2019-11-18T10:46:53Z

+      }
+
+      // Whitespace between variable and colon
+      if (node.name.indexOf(":") !== -1 && !node.params) {


We should improve this check, Scss allow to use : in interpolations and can't have params, so this invalid

alexander-akait · 2019-11-18T10:48:05Z

Please reduce using regexps, it is always bad, if you don't have enough information on node, please open issue in postcss-less instead using hacks

fisker · 2019-11-18T11:06:35Z

@evilebottnawi

https://github.com/shellscape/postcss-less#variables

Note: LESS variables are strictly parsed - a colon (:) must immediately follow a variable name.

postcss-less stop parsing

@foo : bar;

what do we do?

alexander-akait · 2019-11-18T11:10:49Z

@fisker if less support that, we should open issue and fix problem in parser

fisker · 2019-11-18T13:24:52Z

@evilebottnawi shellscape/postcss-less#101 (comment) It was fixed in 2, but changed again in 3 with breaking change shellscape/postcss-less#113 , I don't think they are going to parse it

alexander-akait · 2019-11-18T13:39:57Z

@fisker we should investigate how Less interpreted this, if parser don't parse that propertly we can't update parser until it will be fixed

lydell · 2019-11-18T20:30:59Z

I'm no Less expert, but it seems to be valid on http://lesscss.org/less-preview/

@foo : bar;

a {
  color: @foo;
}

becomes:

a {
  color: bar;
}

alexander-akait · 2019-11-19T09:39:05Z

@fisker it is mean we can't update parser while it is not fixed

thorn0 · 2019-11-19T10:09:41Z

But it works!

Prettier pr-6981
Playground link

--parser less

Input:

@foo : bar;

Output:

@foo: bar;

fixes prettier#7741

fixes #7741

fisker commented Nov 17, 2019

View reviewed changes

fisker force-pushed the update-postcss-less-3 branch from 873ea31 to 754c3cc Compare November 17, 2019 07:01

fisker mentioned this pull request Nov 17, 2019

Bump postcss-less from 1.1.5 to 3.0.2 #6847

Closed

fisker mentioned this pull request Nov 17, 2019

Enable replace-array-includes-with-indexof for all bundles #6982

Merged

4 tasks

fisker force-pushed the update-postcss-less-3 branch from 754c3cc to 6b3456c Compare November 17, 2019 13:23

fisker commented Nov 17, 2019

View reviewed changes

Comment thread scripts/build/babel-plugins/replace-array-includes-with-indexof.js Outdated

fisker mentioned this pull request Nov 17, 2019

Update includes -> indexof plugin #6985

Merged

4 tasks

fisker force-pushed the update-postcss-less-3 branch from ace46c2 to 7b4cf56 Compare November 17, 2019 17:25

This was referenced Nov 17, 2019

Less: passing ruleset with comment to mixin call duplicates the closing semicolon of mixin call #3096

Closed

important rule in LESS mixin moves !important declaration outside mixin #3544

Closed

alexander-akait requested changes Nov 18, 2019

View reviewed changes

fisker added 20 commits February 27, 2020 13:48

fix css_case

9f05b86

fix less

d43be21

fix

f7d65ea

clean

0fb8884

clean

a230e3a

fix nest

4c88599

spaces

8117419

clean

b88d2ed

comment

c803206

remove includes

d4fbcf8

remove useless delete

d8cd76c

update a LESS case

9a6f525

fix comment spaces

1e0424d

add function

0783475

Remove use of '\n'

55e864b

Add parser check

ec35e5b

Add isLessParser

fc77d17

Move lastLineHasInlineComment to utils.js

42930a7

Add 12px & 12PX test

717e6dd

Add &:EXTEND test

73e7ba7

thorn0 force-pushed the update-postcss-less-3 branch from 0ed4d5c to 73e7ba7 Compare February 27, 2020 12:08

thorn0 merged commit acff41e into prettier:next Mar 9, 2020

This was referenced Mar 9, 2020

Close issues fixed by updating postcss-less to v3 (#6981) and write a changelog entry #7741

Closed

Less: ::before becomes : : before in detached rulesets #5791

Closed

thorn0 added a commit to thorn0/prettier that referenced this pull request Mar 10, 2020

add changelog entry for prettier#6981

58a8d43

fixes prettier#7741

thorn0 added a commit to thorn0/prettier that referenced this pull request Mar 10, 2020

add changelog entry for prettier#6981

6f34266

fixes prettier#7741

thorn0 added a commit that referenced this pull request Mar 10, 2020

add changelog entry for #6981 (#7747)

a7e9262

fixes #7741

fisker deleted the update-postcss-less-3 branch March 17, 2020 02:51

fisker mentioned this pull request Mar 24, 2020

Incorrect format in css when selector attribute value is a URL #7850

Closed

github-actions Bot locked as resolved and limited conversation to collaborators Mar 18, 2021

Uh oh!

Conversation

fisker commented Nov 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lydell commented Nov 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fisker commented Nov 17, 2019

Uh oh!

thorn0 commented Nov 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fisker commented Nov 17, 2019

Uh oh!

fisker commented Nov 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fisker commented Nov 17, 2019

Uh oh!

thorn0 commented Nov 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thorn0 commented Nov 17, 2019

Uh oh!

fisker commented Nov 18, 2019

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait Nov 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait commented Nov 18, 2019

thorn0 commented Nov 17, 2019 •

edited

Loading

fisker commented Nov 17, 2019 •

edited

Loading

thorn0 commented Nov 17, 2019 •

edited

Loading

alexander-akait Nov 18, 2019 •

edited

Loading

fisker commented Nov 18, 2019 •

edited

Loading

alexander-akait commented Nov 18, 2019 •

edited

Loading

fisker commented Nov 18, 2019 •

edited

Loading

alexander-akait commented Nov 19, 2019 •

edited

Loading