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

Fix parsing of !important #119

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@jwilsson
Collaborator

jwilsson commented Sep 29, 2018

Which issue # if any, does this resolve?

#118

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.

@jwilsson jwilsson changed the title from Issue 118 to Fix parsing of !important Sep 29, 2018

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Sep 29, 2018

Codecov Report

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

Impacted file tree graph

@@            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.

codecov-io commented Sep 29, 2018

Codecov Report

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

Impacted file tree graph

@@            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.

@@ -13,18 +15,7 @@ module.exports = {
return parser.root;
},
stringify(node, builder) {

This comment has been minimized.

@shellscape

shellscape Sep 29, 2018

Owner

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

@shellscape

shellscape Sep 29, 2018

Owner

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

This comment has been minimized.

@jwilsson

jwilsson Sep 30, 2018

Collaborator

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.

@jwilsson

jwilsson Sep 30, 2018

Collaborator

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.

This comment has been minimized.

@shellscape

shellscape Sep 30, 2018

Owner

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

@shellscape

shellscape Sep 30, 2018

Owner

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

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

This comment has been minimized.

@shellscape

shellscape Sep 29, 2018

Owner

same with this

@shellscape

shellscape Sep 29, 2018

Owner

same with this

This comment has been minimized.

@jwilsson

jwilsson Sep 30, 2018

Collaborator

Same as above.

@jwilsson

jwilsson Sep 30, 2018

Collaborator

Same as above.

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

This comment has been minimized.

@shellscape

shellscape Sep 29, 2018

Owner

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.

@shellscape

shellscape Sep 29, 2018

Owner

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.

This comment has been minimized.

@jwilsson

jwilsson Sep 30, 2018

Collaborator

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?

@jwilsson

jwilsson Sep 30, 2018

Collaborator

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?

@@ -61,3 +61,11 @@ test('mixin with !important', async (t) => {
const result = run(less);
t.is(result, less);
});
test('AtRule custom stringifier', async (t) => {

This comment has been minimized.

@shellscape

shellscape Sep 29, 2018

Owner

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

@shellscape

shellscape Sep 29, 2018

Owner

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

This comment has been minimized.

@jwilsson

jwilsson Sep 30, 2018

Collaborator

Gotcha!

@jwilsson

jwilsson Sep 30, 2018

Collaborator

Gotcha!

@@ -175,6 +175,34 @@ test('!important, no whitespace', (t) => {
t.is(nodeToString(root), less);
});
test('!important, whitespace between', (t) => {
const less = `
.foo()! important;

This comment has been minimized.

@shellscape

shellscape Sep 29, 2018

Owner

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.

@shellscape

shellscape Sep 29, 2018

Owner

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.

This comment has been minimized.

@jwilsson

jwilsson Sep 30, 2018

Collaborator

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.

@jwilsson

jwilsson Sep 30, 2018

Collaborator

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) {

This comment has been minimized.

@shellscape

shellscape Sep 29, 2018

Owner

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

@shellscape

shellscape Sep 29, 2018

Owner

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

This comment has been minimized.

@jwilsson

jwilsson Sep 30, 2018

Collaborator

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.

@jwilsson

jwilsson Sep 30, 2018

Collaborator

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.

@@ -26,6 +27,7 @@ module.exports = class LessParser extends Parser {
super.atrule(token);
importNode(this.lastNode);
variableNode(this.lastNode);
toString(this.lastNode);

This comment has been minimized.

@shellscape

shellscape Sep 29, 2018

Owner

I don't understand why this was done

@shellscape

shellscape Sep 29, 2018

Owner

I don't understand why this was done

This comment has been minimized.

@jwilsson

jwilsson Sep 30, 2018

Collaborator

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

@jwilsson

jwilsson Sep 30, 2018

Collaborator

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

@@ -125,18 +119,19 @@ module.exports = class LessParser extends Parser {
this.lastNode.mixin = true;
this.lastNode.raws.identifier = identifier;
// const importantIndex = tokens.findIndex((t) => importantPattern.test(t[1]));
const { params } = this.lastNode;
const importantMatch = params.match(importantPattern);

This comment has been minimized.

@shellscape

shellscape Sep 29, 2018

Owner

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

@shellscape

shellscape Sep 29, 2018

Owner

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

This comment has been minimized.

@jwilsson

jwilsson Sep 30, 2018

Collaborator

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

@jwilsson

jwilsson Sep 30, 2018

Collaborator

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

@shellscape

This comment has been minimized.

Show comment
Hide comment
@shellscape

shellscape Sep 30, 2018

Owner

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)

Owner

shellscape commented Sep 30, 2018

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment