-
-
Notifications
You must be signed in to change notification settings - Fork 322
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
cssDeclarationSorter is not safe when shorthand after longhand property #535
Comments
Looks bug, feel free to fix, don't have time right now 😞 |
I am not sure, that css-declaration-sorter is safe. Seems like it was designed to use manually by the developer. And could be dangerous to run it on legacy code. |
Maybe we should disable it in |
@ai in theory it is dangerous only in some cases where using idents, maybe we can handle this in safe way |
@evilebottnawi it can be dangerous if the code is written wrong. Like in this example (shorthand override longhand). Yeap, we could make What do you think if we will disable |
@ai feel free to do this and release patch version |
I started to moving it, but can’t fix some tests #536 |
👋 chiming in, a straightforward fix would be to keep the |
@Siilwyn yeap, but we need to keep order of any longhand/shorthand properties |
same problem when using Input:
Output:
And Expected output:
|
@kingback do you include prefixed properties in your source code? Regarding the fixed order I think I'll have time to look into it tomorrow! |
yes, some legacy code use prefixed properties to compatible with old mobile devices, maybe we can remove the vendor prefix before sort properties |
Still a valid use-case, @kingback I would recommend to look at using autoprefixer. But I'll take a look at prefixed properties too. |
Given the amount of issues we have around |
@andyjansson i think we should fix it, don't disable, can we provide list of problems here? |
Well the issue is that it's entirely too naïve and doesn't respect precedence. I do not believe we can easily fix it without major rewrites, which is hard given that it is an external dependency. |
@andyjansson we can move sort algorithm inside |
People, you are killing cssnano! ((( |
@dutchenkoOleg That's an unwarranted response. What gives? |
Yes I'm certain I can make this work, just need to put time and thoughts into the problem. Doesn't really matter it is external or not right? I'm willing to change parts on my end. :) |
@andyjansson #535 (comment) and next one |
@dutchenkoOleg and how are we "killing cssnano"? Like @ai says, If you have any concerns, go ahead and voice them, but we can do without the outburts and hyperboles. |
I think you needed to turn off this plug-in immediately, when you realized that this is unsafe, rather than discuss it "...to be or not to be..." |
agree with @andyjansson, not only this issue, there're issues like postcss-calc/issues/48 makes me feel like it's unsafe to use
So we can have a choice to deside which plugin we don't want to use. |
We've always supported that. https://cssnano.co/guides/presets#excluding-transforms
The issue here is @ai do you have access to this repo? If not, other options are to fork the code, or move the logic to |
That depends on what you compare it with. It's a shorthand of Whether it's a shorthand or not isn't really the key issue here; precedence is. The order of declaration makes a huge semantic difference, regardless of whether shorthand or longhand declarations are involved.
The above two snippets, although being identical in what properties they contain, mean two very different things due to the order they're declared in. |
Another week ended. Did we released any solution or we are still in political powerless? It is a critical issue. We had a quick option and long option. Why we prefer to choose the worst option to do nothing? /cc @evilebottnawi |
@ai I apologize for the current situation, but I really have a lot of work and I will not have enough time 😞 So I gave github/npm access to you (@ai) and @andyjansson so feel free to release, i can find time only tomorrow. |
@Siilwyn you solution is not good. Why? It is bad to have list of properties in file https://github.com/cssnano/cssnano/pull/580/files#diff-bc4bbe1119837b527465f18f074304ff (each new property require adding to file). A lot of changes not related to PR. Example: body{font-size:100%;line-height:1.5} To body{line-height:1.5;font-size:100%} Why it is happens? It is not alphabetic order. We need maximum alphabetic order for better Here issue about new option Siilwyn/css-declaration-sorter#64 to fix this bug. Main idea order all property in |
Would it be possible to at least update the optimisations docs: to indicate that this plugin is unsafe and have it off by default? Recently updated to a newer version of cssnano in a legacy project and this threw me for a loop. I'll make a PR for that if anyone wants. This is not safe, especially when dealing with a CSS pre-processor. |
I will try to release it right now |
Please provide example. |
@evilebottnawi I realize now I may have not worded that the best. It's not anything intrinsic to the pre-processor, but more so that it makes it harder to reason about things like mixins: mixins.less .mixin-a {
background-color: red;
} someotherfile.less .selector-a {
.mixin-a;
background: blue;
} Output: .selector-a {
background: blue;
background-color: red;
} Since there is a higher level of abstraction, it's a bit easier to shoot yourself in the foot. |
@dkenul don't use |
@evilebottnawi I know that, it's not ideal but it is still much better than disabling the plugin! That is why I proposed this as the solution for now: #535 (comment) Please reconsider, I would hate it if all these hours I spend on a solution I proposed and worked on to be wasted. :( |
@evilebottnawi can you elaborate? This specific issue can occur with or without a pre-processor. |
I released 4.1.1 without css sorter |
@dkenul please create new issue with example what you have in |
What about the alternative where we maintain a collection of shorthand and their longhand expansions? It will still require vigilance to keep up to date but should be less change overall. Similar to https://github.com/stylelint/stylelint/blob/master/lib/reference/shorthandData.js Perhaps also expose it as an option too to allow people to customise it such that new additions can be added outside of releases. |
@jordrake sounds good |
@jordrake I already keep a collection of properties up-to-date at |
Fixed #855 |
Input:
Output:
Expected output:
/cc @evilebottnawi
Other bug (need add tests):
Input:
Output:
Other bug (need also add tests):
Input:
Output:
The text was updated successfully, but these errors were encountered: