postcss-selector-parser icon indicating copy to clipboard operation
postcss-selector-parser copied to clipboard

invalid/non stable ast for combinator with comment

Open alexander-akait opened this issue 6 years ago • 23 comments

Input:

h2 /*test*/ h4 { } /* Here we don't have `comment` node in ast (store comment in `raws`) */
h2/*test*/h4 { } /* Comment in ast */

It is do impossible analyzes comments, also adopt new version to stylelint and prettier. For me it is bug, but fixing this change ast.

alexander-akait avatar Mar 15 '19 14:03 alexander-akait

/cc @ai what do you think about this

alexander-akait avatar Mar 15 '19 14:03 alexander-akait

Why it will change AST?

ai avatar Mar 15 '19 19:03 ai

@ai new comment node appears in ast(before it was in raws), just want to clarify should we release this as patch or major?

alexander-akait avatar Mar 16 '19 13:03 alexander-akait

Technically it should be major

ai avatar Mar 16 '19 16:03 ai

Same problem for [ /*t*/ title /*t*/ = /*t*/ "Something" /*t*/ ], comments should be part of ast not raw

alexander-akait avatar Mar 19 '19 15:03 alexander-akait

/cc @ai i ahve some questions about spaces too Example:

[   href   =   "test"   i   ] { }
  ^      ^   ^        ^   ^

What node(s) should store spaces in this case (inside raws?)? Because now logic in parser is very misleading and I can't understand whether we do it right or not. We have attribute, value, insensitive values.

alexander-akait avatar Mar 19 '19 15:03 alexander-akait

To be honest, I am not sure that I know the best answer.

But I think in selector parser we should work with spaces in a different way, compare to PostCSS core. In main CSS syntax, whitespaces don’t mean anything. In selector, whitespace can have a lot of meanings.

So, I think it could be better to have a special token for spaces.

ai avatar Mar 19 '19 16:03 ai

@ai yep, we already have special token for space, my questions about where we should store meta information about space (raws). Let's see on example above:

  • First spaces should be stored in raws.before or raws.attribute.before?
  • Second space should be stored in raws.attribute.before or raws.operatore.before?
  • Third space should be stored in raws.operator.after or raws.value.before
  • Four space should be stored in raws.value.after or raws.insensitive.before (insensitive will be renamed in flag in next major)
  • Five space should be stored in raws.insensitive.after or raws.after (insensitive will be renamed in flag in next major)

alexander-akait avatar Mar 19 '19 16:03 alexander-akait

Why do we need a raws for whitespaces when we have space token?

ai avatar Mar 19 '19 16:03 ai

@ai hm can you provide example/PoC of ast with space token (based on example above)? We need this spaces for linting/printing in stylelint and prettier

alexander-akait avatar Mar 19 '19 16:03 alexander-akait

a b > { type: "word", value: "a" }, { type: "space", value: " " }, { type: "word", value: "b" }

ai avatar Mar 19 '19 16:03 ai

@ai in example above space is descendant selector, we already provide combinator as separate node, but in my example spaces mean nothing so we omit them from ast

alexander-akait avatar Mar 19 '19 16:03 alexander-akait

I am not sure that we should omit them from AST.

ai avatar Mar 19 '19 16:03 ai

@ai I do not see their meaning as they really do not carry here any semantic load, also we do this right now, we can plan this on next major, but will be great solve problems with spaces using raws for next release

alexander-akait avatar Mar 19 '19 16:03 alexander-akait

Yeap. This discussion is on you. Sorry, that I can't help right now.

ai avatar Mar 19 '19 17:03 ai

@ai Maybe not related to issue, but what is blocker integrate selector parser in postcss? Non standard syntax? Or nobody send a PR?

alexander-akait avatar Mar 20 '19 10:03 alexander-akait

@evilebottnawi I am afraid that we will freeze API which will not be the best.

ai avatar Mar 20 '19 15:03 ai

@ai we can afraid this forever :smile: To be honestly ast of postcss is not enough for prettier/stylelint nowadays (prettier breaks code in many cases and i recommended don't use this for css/scss/less/etc). I think we should start integration selector/value/media parsers otherwise we will need do fork postcss and continue development :disappointed:

alexander-akait avatar Mar 21 '19 10:03 alexander-akait

Also we can release this under flag/option and as experimental (adding information to readme about non stable)

We have big tests code base in stylelint and prettier and webpack so i think we catch all problems very fast

alexander-akait avatar Mar 21 '19 12:03 alexander-akait

RIght now our main focus is visitor API https://github.com/postcss/postcss/pull/1245

ai avatar Mar 21 '19 15:03 ai

Great feature :+1: Already look on this.

@ai Is there any sense send a PR or better fork postcss and starting own development for prettier? Because i don't want to waste my time on something what will be never merged.

alexander-akait avatar Mar 21 '19 16:03 alexander-akait

@evilebottnawi I have a better plan. Let’s release new AST in postcss-selector-parser and if nobody will complain about it, we can move it to PostCSS

ai avatar Mar 21 '19 16:03 ai

@ai okay :+1: i will ping you before release or when i have some questions

alexander-akait avatar Mar 21 '19 16:03 alexander-akait