Fix parsing of !important #119

jwilsson · 2018-09-29T09:26:10Z

Which issue # if any, does this resolve?

Please check one:

New tests created for this change
Tests updated for this change

This PR:

Adds new API
Extends existing API, backwards-compatible
Introduces a breaking change
Fixes a bug

While working on a fix for #118, I also discovered that mixin AtRules wouldn't be properly stringified, so I performed a little refactor, moving some things around to get that to work properly too.

codecov-io · 2018-09-29T09:28:44Z

Codecov Report

Merging #119 into master will increase coverage by 0.33%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #119      +/-   ##
==========================================
+ Coverage   95.74%   96.07%   +0.33%     
==========================================
  Files           8       11       +3     
  Lines         188      204      +16     
==========================================
+ Hits          180      196      +16     
  Misses          8        8

Impacted Files	Coverage Δ
lib/nodes/nodeToString.js	`100% <100%> (ø)`
lib/nodes/toString.js	`100% <100%> (ø)`
lib/LessParser.js	`98.73% <100%> (+0.03%)`	⬆️
lib/nodes/stringify.js	`100% <100%> (ø)`
lib/LessStringifier.js	`88.88% <100%> (+1.93%)`	⬆️
lib/index.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f444c9...194651e. Read the comment docs.

shellscape · 2018-09-29T14:24:14Z

lib/index.js

    return parser.root;
  },

-  stringify(node, builder) {


I don't understand why this was moved into a separate file.

I ran into some issues with cyclic dependencies when trying to require it from index.js, but I'll see if I can find another way around it.

OK I'd need to know how to reproduce that error. Let's address that in a separate issue.

shellscape · 2018-09-29T14:24:36Z

lib/index.js

-    stringifier.stringify(node);
-  },
-
-  nodeToString(node) {


same with this

Same as above.

shellscape · 2018-09-29T14:25:52Z

lib/nodes/nodeToString.js

@@ -0,0 +1,13 @@
+const { stringify } = require('./stringify');


this nodes directory is here for utils/helpers with parsing particular node types, so that's not the right home for this file, nor stringify.js.

I'll try and find a way around it as per above, but otherwise, do you want it in a new folder utils (or similar) or just in the root of lib?

shellscape · 2018-09-29T14:27:25Z

test/stringify.test.js

  t.is(result, less);
 });
+
+test('AtRule custom stringifier', async (t) => {


needs a more apt description: "spaced ! important" would do

shellscape · 2018-09-29T14:28:58Z

test/parser/mixins.test.js


+test('!important, whitespace between', (t) => {
+  const less = `
+                    .foo()! important;


I haven't been able to find documentation in or out of the spec that says this form is valid (with no leading space before the !) and postcss doesn't parse that with standard declarations. I know this was part of the last major version, but it was intentionally left out of the current major because there wasn't any docs I could find.

Hmm, I'm getting back valid results from both postcss, the official less compiler, as well as browsers when omitting the leading space. I guess the closest to a spec on it would be https://www.w3.org/TR/css-syntax-3/#declaration-diagram but I agree that it's not superclear.

shellscape · 2018-09-29T14:30:09Z

lib/LessStringifier.js

-    const important = node.raws.important || '';
+    let important = '';
+
+    if (node.important && node.raws.important) {


I don't understand this addition. If a node is important, then node.raws.important should always exist?

I'll try to explain as good as I can cause this one is a bit funny :)

postcss will only include the !important part in node.raws if it's not !important (with a leading space and all lowercase, GitHub removed the leading space). Everything else, a space between ! and important, one or more chars uppercase, etc. will create a node.raws property. That's why I made the same check here.

shellscape · 2018-09-29T14:33:16Z

lib/LessParser.js

    super.atrule(token);
    importNode(this.lastNode);
    variableNode(this.lastNode);
+    toString(this.lastNode);


I don't understand why this was done

I was trying to extend AtRule nodes with our own toString, but I'm hoping I can find a better solution.

shellscape · 2018-09-29T15:09:12Z

lib/LessParser.js


-      // const importantIndex = tokens.findIndex((t) => importantPattern.test(t[1]));
+      const { params } = this.lastNode;
+      const importantMatch = params.match(importantPattern);


I think I understand why you're looking at the params to detect important here, but I believe that's going to open up the parser to future frailty. particularly when it comes to nested rulesets containing a declaration that has !important - e.g. https://github.com/shellscape/postcss-less/blob/master/test/parser/mixins.test.js#L178

the Right Way ™️ to tackle this is by inspecting the tokens

You're right, I'll take a closer look at inspecting the tokens.

shellscape · 2018-09-30T14:11:45Z

So here's the token breakdown of the different forms of !important:

.foo()!important;
[ [ 'word', '.foo', 1, 1, 1, 4 ],
  [ 'brackets', '()', 1, 5, 1, 6 ],
  [ 'word', '!important', 1, 7, 1, 16 ],
  [ ';', ';', 1, 17 ] ]

.foo()! important;
[ [ 'word', '.foo', 1, 1, 1, 4 ],
  [ 'brackets', '()', 1, 5, 1, 6 ],
  [ 'word', '!', 1, 7, 1, 7 ],
  [ 'space', ' ' ],
  [ 'word', 'important', 1, 9, 1, 17 ],
  [ ';', ';', 1, 18 ] ]

.foo() ! important;
[ [ 'word', '.foo', 1, 1, 1, 4 ],
  [ 'brackets', '()', 1, 5, 1, 6 ],
  [ 'space', ' ' ],
  [ 'word', '!', 1, 8, 1, 8 ],
  [ 'space', ' ' ],
  [ 'word', 'important', 1, 10, 1, 18 ],
  [ ';', ';', 1, 19 ] ]
                                                                                                                               
.foo() !important;
[ [ 'word', '.foo', 1, 1, 1, 4 ],
  [ 'brackets', '()', 1, 5, 1, 6 ],
  [ 'space', ' ' ],
  [ 'word', '!important', 1, 8, 1, 17 ],
  [ ';', ';', 1, 18 ] ]

Since unknownWord gets the entire token set for the unknown word, and we can inspect that token set for important, the solution here is to simply look for a word token that equals ! and then do a short loop that adds each following space token to an array, until a word token of important is reached. once it's reached, we have the raws value by reducing the array, and we also know which elements to remove from the tokens array for further processing. if it's never reached, we don't have an important node.

so while your original solution was partially effective, the easiest route here is probably to reset and apply that algorithm. (and please do delete the commented code I had in there, that's my baddie, and left over from a faulty attempt on important)

(and forgot to mention - if you need a hand with implementing this, please let me know)

jwilsson · 2018-11-05T20:37:32Z

Superseded by #124.

jwilsson added 2 commits September 29, 2018 11:10

Parse all forms of !important

ec47450

Add custom stringifier for AtRules

194651e

jwilsson changed the title ~~Issue 118~~ Fix parsing of !important Sep 29, 2018

shellscape reviewed Sep 29, 2018

View reviewed changes

jwilsson mentioned this pull request Nov 5, 2018

Fix parsing of !important - take 2 #124

Closed

6 tasks

jwilsson closed this Nov 5, 2018

shellscape mentioned this pull request Nov 14, 2018

fix: proposed token important fix #126

Merged

6 tasks

jwilsson deleted the issue-118 branch November 14, 2018 19:36

		@@ -0,0 +1,13 @@
		const { stringify } = require('./stringify');

Uh oh!

Fix parsing of !important #119

Fix parsing of !important #119

Uh oh!

Conversation

jwilsson commented Sep 29, 2018

Uh oh!

codecov-io commented Sep 29, 2018

Codecov Report

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!

jwilsson Sep 30, 2018 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

shellscape commented Sep 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jwilsson commented Nov 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jwilsson Sep 30, 2018 •

edited

Loading

shellscape commented Sep 30, 2018 •

edited

Loading