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 - take 2 #124

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@jwilsson
Collaborator

jwilsson commented Nov 5, 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

This should be a better approach to it than #119, the logic is mostly from core postcss. I'm hoping to find a way to consolidate it to one solution across both libraries.

@@ -24,7 +30,7 @@ module.exports = class LessStringifier extends Stringifier {
if (node.nodes) {
this.block(node, name + params + important);
} else {
const end = (node.raws.between || '') + important + (semicolon ? ';' : '');
const end = important + (semicolon ? ';' : '') + (node.raws.between || '');

This comment has been minimized.

@jwilsson

jwilsson Nov 5, 2018

Collaborator

I'm not 100% satisfied with this. There's one stringify test ("mixin with !important") that's failing without it so I thought I'd leave it in for the sake of discussion.

I haven't been able to find any downsides to it yet, though. But let me know what you think and we'll take it from there.

@jwilsson jwilsson referenced this pull request Nov 5, 2018

Closed

Fix parsing of !important #119

2 of 6 tasks complete
const important = node.raws.important || '';
let important = '';
if (node.important && node.raws.important) {

This comment has been minimized.

@shellscape

shellscape Nov 5, 2018

Owner

maybe I'm not reading the code above correctly, but in what scenario will node.raws.important be populated when node.important is falsy?

This comment has been minimized.

@jwilsson

jwilsson Nov 6, 2018

Collaborator

True, that check can be simplified.

@jwilsson

This comment has been minimized.

Collaborator

jwilsson commented Nov 7, 2018

I've simplified the !important check but CI is failing due to something else (something in the config file I guess).

@shellscape

This comment has been minimized.

Owner

shellscape commented Nov 13, 2018

@jwilsson there's a typo in the circle ci configs that I used across most of my repos, and I've had to incrementally clean it. Circle used to permit the use of duplicate version keys in the yaml, but introduced a "fix" that started throwing errors. I'll update that on master, and you can merge master to resolve it and get CI working again.

@codecov-io

This comment has been minimized.

codecov-io commented Nov 13, 2018

Codecov Report

Merging #124 into master will increase coverage by 0.31%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #124      +/-   ##
==========================================
+ Coverage   95.74%   96.05%   +0.31%     
==========================================
  Files           8        8              
  Lines         188      203      +15     
==========================================
+ Hits          180      195      +15     
  Misses          8        8
Impacted Files Coverage Δ
lib/LessParser.js 98.91% <100%> (+0.21%) ⬆️

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 3016597...cbba418. Read the comment docs.

@shellscape

This comment has been minimized.

Owner

shellscape commented Nov 13, 2018

So the tests are exactly what we needed. I wasn't quite sure about the direction of your solution though. I've been trying to follow the mantra that the PostCSS maintainers have shifted towards, in getting the tokens array setup before parsing/processing methods take over. It's a little different than string examination post-process. Here's my proposal to go the tokens route:

const importantTokens = [];
for (const token of tokens) {
if (token[1] === '!' || importantTokens.length) {
importantTokens.push(token);
}
if (token[1] === 'important') {
break;
}
}
if (importantTokens.length) {
const [bangToken] = importantTokens;
const bangIndex = tokens.indexOf(bangToken);
const last = importantTokens[importantTokens.length - 1];
const start = [bangToken[2], bangToken[3]];
const end = [last[4], last[5]];
const combined = importantTokens.map((t) => t[1]).reduce((a, c) => a + c);
const newToken = ['word', combined].concat(start, end);
tokens.splice(bangIndex, importantTokens.length, newToken);
}

Granted it could probably use a little polish, but it passes the tests you've added on this PR, and doesn't need Stringifier modification. Thoughts?

@jwilsson

This comment has been minimized.

Collaborator

jwilsson commented Nov 13, 2018

@shellscape Cool! I'd love to learn some more about it. Have you seen any blog post, issue comment, etc. from the PostCSS maintainers? Or is the best way to learn more just reading the PostCSS code more closely?

I left one small comment on your solution, but otherwise I think it looks great! Especially since we don't need any stringifier changes.

Edit: Forgot to mention, feel free to close this one and merge your changes as you see fit.

@shellscape

This comment has been minimized.

Owner

shellscape commented Nov 14, 2018

OK, cool. I didn't want to go opening a new PR with this one pending until you had a chance to comment on the alternative direction. This stuff is no joke to work on and I'd bet you had some time into yours.

With regard to following the PostCSS methods; I gleaned that stuff from the code itself. There were some issues for the last major that went into some detail in comments about the token-first parsing deal as well. Nothing official, but just absorbed. Picked up most of that while doing the last rewrite to support v7. FWIW I'm hoping to go the same route with postcss-values-parser because it's so much easier to maintain going that route. Parsers are hard!

On your suggestion, closing this one and will open a new PR on the other branch.

@shellscape shellscape closed this Nov 14, 2018

@shellscape shellscape referenced this pull request Nov 14, 2018

Merged

fix: proposed token important fix #126

3 of 6 tasks complete

@jwilsson jwilsson deleted the issue-118-take-2 branch Nov 14, 2018

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