Use PostCSS 7 #112
Use PostCSS 7 #112
Conversation
|
@ai please describe the breaking changes. |
lib/parser.js
Outdated
| @@ -0,0 +1,507 @@ | |||
| /* eslint-disable one-var */ | |||
There was a problem hiding this comment.
this introduces an incredible amount of code that this library must now maintain. definitely not preferable. why is this necessary when moving to PostCSS@7?
There was a problem hiding this comment.
Because PostCSS 6+ uses different tokenizer. So postcss/lib/parser became completely different.
There was a problem hiding this comment.
OK. I would rather an update adapt to the changes in the tokenizer, or a compatible parser be made a separate module that others could use, than to introduce 507 new lines of code and a tokenizer to this module. There's already quite a barrier to outside contributors in this module, and the proposed addition is just going to make that barrier more difficult as well.
There was a problem hiding this comment.
OK, let's release it first and in next version you will rewrite parser when you have a time.
What do you think about it?
There was a problem hiding this comment.
That's not really an optimal solution. I'm not in the habit of intentionally taking on tech debt. Especially since a secondary parser to backfill PostCSS's parser changes isn't something I'm interested in maintaining moving forward. That work should be done ahead of time and separately from this module.
There was a problem hiding this comment.
I'm not generally concerned about the size of node_modules - and I'm not part of the crowd that obsesses over that metric. Bundle sizes would not be effected unless poor choices were made in the separate package and/or tree-shaking was not used. That's not a compelling argument for including a 500 line parser in an otherwise small package.
There was a problem hiding this comment.
That's not a compelling argument for including a 500 line parser in an otherwise small package.
So what use case will work worse with this 500 LOC? The last commit was at May 30, how many changes you plan to do so worry about code size?
We can’t rewrite the whole parser since it will require few days and you will not do it.
We can’t move forke the parser since it will force to change dependencies in too many projects.
We can’t keep it, since many plugins will not compatible with this parser (your parser generate nodes without necessary methods).
We can release lib/parser.js to separated npm package. But what it will change? What will be a difference if this code will be in postcss-less-subparser?
As a creator of PostCSS I need to take care of the ecosystem. You are the only PostCSS 5 project right now. We need to solve it somehow. I even spent my Sunday to fix it for you.
What solution do you wee as a maintainer?
There was a problem hiding this comment.
I can spend few extra hours and remove code, Wich you override in Less parser to reduce 500 LOC to something around 300 LOC.
Will it be OK?
There was a problem hiding this comment.
I'm not sure what you're trying to say. I see some things in there that might be trying to shame me into accepting the PR - not sure if that was the intent, but it's not really motivating.
As a creator of PostCSS I need to take care of the ecosystem.
Sure.
You are the only PostCSS 5 project right now.
And that's OK.
We need to solve it somehow.
The parser that was being relied upon was drastically changed between PostCSS@5 and PostCSS@7. There needs to be a compatibility layer to bridge the gap. That layer doesn't belong in this module.
I even spent my Sunday to fix it for you.
That's very generous of you. However, if you had started work and realized it would be a large change, it would have been prudent at that point to discuss a large change before spending the time on it - I'm pretty sure that's standard practice for most projects.
I can spend few extra hours and remove code, Wich you override in Less parser to reduce 500 LOC to something around 300 LOC. Will it be OK?
I apologize - I don't know what you're trying to convey in that reply.
There was a problem hiding this comment.
it's not really motivating.
Yeap, maybe I am overreacted.
But it is the same from your side. You just say “no” without really trying to find a way to solve it.
It is really not motivating me as well. It was really bad feedback for PR.
Let’s say sorry to each other and start again.
Here is my basement:
- No one has time to rewrite the parser.
- We need a release ASAP.
Here are solutions that I can see:
- I can reduce the compatibility layer size. Like in
lib/less-parseryou override many methods, we can remove it. - We can release compatibility layer to npm package. But this package will not be supported very well since it will be used only in the single package.
- You can give maintenance for me and I just release it as it.
What is your basement and what option do you see?
|
Hm, I was wrong. There is no breaking changes in PostCSS AST between 5 and 7. We added only new methods. |
|
@shellscape I rewrite PR. Now it adds only 200 LOC. No ESLint changes. No |
|
I changed the base branch for this PR to the |
|
Sure, if you think you will do it, I am OK with it. I just need to have the same AST API across ecosystem. Don’t care who and how will write a code for it :). |
|
The main difference of PostCSS 7 parser is that we do not have tokens array anymore. In 7.0 parser ask tokenizer to return next token, process this token, then ask tokenizer to return next token, etc. As result, we do not store a big array of tokens in the memory between tokenizing and parsing steps. It allows parsing very big CSS files without memory limit error. |
|
Sounds like this lib needs a complete refactor to utilize the new tokenizer. |
|
@shellscape yeap ;). I rewrote PostCSS tokenizer/parser in few days. It is not hard, but require a lot of routing fixes. If you have this time you can do it again. Or just use the compatibility layout. I think it is very rare to parse huge Less files. |
Which issue # if any, does this resolve?
Fixes #85
Please check one:
This PR: