Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
| return parser.root; | ||
| }, | ||
|
|
||
| stringify(node, builder) { |
There was a problem hiding this comment.
I don't understand why this was moved into a separate file.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
OK I'd need to know how to reproduce that error. Let's address that in a separate issue.
| stringifier.stringify(node); | ||
| }, | ||
|
|
||
| nodeToString(node) { |
| @@ -0,0 +1,13 @@ | |||
| const { stringify } = require('./stringify'); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
| t.is(result, less); | ||
| }); | ||
|
|
||
| test('AtRule custom stringifier', async (t) => { |
There was a problem hiding this comment.
needs a more apt description: "spaced ! important" would do
|
|
||
| test('!important, whitespace between', (t) => { | ||
| const less = ` | ||
| .foo()! important; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| const important = node.raws.important || ''; | ||
| let important = ''; | ||
|
|
||
| if (node.important && node.raws.important) { |
There was a problem hiding this comment.
I don't understand this addition. If a node is important, then node.raws.important should always exist?
There was a problem hiding this comment.
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.
| super.atrule(token); | ||
| importNode(this.lastNode); | ||
| variableNode(this.lastNode); | ||
| toString(this.lastNode); |
There was a problem hiding this comment.
I don't understand why this was done
There was a problem hiding this comment.
I was trying to extend AtRule nodes with our own toString, but I'm hoping I can find a better solution.
|
|
||
| // const importantIndex = tokens.findIndex((t) => importantPattern.test(t[1])); | ||
| const { params } = this.lastNode; | ||
| const importantMatch = params.match(importantPattern); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
You're right, I'll take a closer look at inspecting the tokens.
|
So here's the token breakdown of the different forms of .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 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) |
|
Superseded by #124. |
Which issue # if any, does this resolve?
#118
Please check one:
This PR:
While working on a fix for #118, I also discovered that mixin
AtRuleswouldn't be properly stringified, so I performed a little refactor, moving some things around to get that to work properly too.