Upgrade css declaration sorter #855

Siilwyn · 2019-12-12T15:58:44Z

🎐 A fresh take on PR #580 to resolve issue #535 making css-declaration-sorter safe by adding a new option keepOverrides.

For now I'm using the GitHub repository to get some feedback on this PR, once it's reviewed I'll publish the changes as a new major version 5.0.

I do need some help to get all tests working, I did update the snapshots but can't get the integration tests to pass.

packages/cssnano-preset-default/package.json

codecov-io · 2019-12-12T18:22:28Z

Codecov Report

Merging #855 into master will decrease coverage by 0.05%.
The diff coverage is 96.87%.

@@            Coverage Diff             @@
##           master     #855      +/-   ##
==========================================
- Coverage   97.33%   97.27%   -0.06%     
==========================================
  Files         118      118              
  Lines        3452     3452              
  Branches     1035     1035              
==========================================
- Hits         3360     3358       -2     
- Misses         84       86       +2     
  Partials        8        8

Impacted Files	Coverage Δ
...ages/postcss-ordered-values/src/rules/animation.js	`100% <ø> (ø)`	⬆️
packages/postcss-minify-params/src/index.js	`100% <ø> (ø)`	⬆️
packages/postcss-reduce-transforms/src/index.js	`100% <ø> (ø)`	⬆️
packages/postcss-merge-idents/src/index.js	`100% <ø> (ø)`	⬆️
packages/postcss-merge-rules/src/index.js	`97.7% <ø> (ø)`	⬆️
packages/cssnano-preset-default/src/index.js	`100% <ø> (ø)`	⬆️
packages/postcss-minify-gradients/src/index.js	`100% <ø> (ø)`	⬆️
...ages/postcss-ordered-values/src/rules/boxShadow.js	`100% <ø> (ø)`	⬆️
...es/postcss-normalize-timing-functions/src/index.js	`65.62% <ø> (ø)`	⬆️
...ges/postcss-ordered-values/src/rules/transition.js	`100% <ø> (ø)`	⬆️
... and 7 more

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 20cd9a7...cd1a871. Read the comment docs.

alexander-akait

We lose yarn lock file, also can we add simple tests?

Siilwyn · 2019-12-17T10:22:04Z

@evilebottnawi tests are here in the package: https://github.com/Siilwyn/css-declaration-sorter/blob/master/tests/test.js
Do you want to add tests to cssnano too?

I've reset the yarn lock files.

Siilwyn · 2020-01-03T16:23:46Z

@evilebottnawi since you did not reply I assumed you want tests in this project too, I have added test with commit d16ccfb.
Please review, thanks!

Siilwyn · 2020-01-11T13:35:17Z

@ben-eb could you review this PR?

Only the framework integration tests are failing even after running yarn build:integration, any help appreciated.

ben-eb · 2020-01-13T23:00:52Z

I'm pretty sure the integration tests work on diffing the whole CSS file, which unfortunately makes it difficult to know what changed. If you have changed the order then it's likely to have affected a lot of the integration tests; but there should be a command you can use to rebuild them and then verify the output in some way.

Ideas on changing the integration tests to make the output more human-readable of course welcomed. 🙂

Siilwyn · 2020-01-14T09:04:36Z

@ben-eb thanks for your reply, what command would rebuild them? As I mentioned yarn build:integration gives the same failing fixture output.

ben-eb · 2020-01-14T21:44:02Z

Ah, sorry I misread that part of your message. Seems strange that running those didn't fix it. 😞

Siilwyn · 2020-01-28T09:53:17Z

@evilebottnawi ready for review.

alexander-akait · 2020-01-28T10:56:38Z

packages/cssnano-preset-advanced/README.md

@@ -106,15 +106,15 @@ Note that you may wish to publish your own preset to npm for reusability, should

 ## Plugins

-### [`autoprefixer`](https://github.com/postcss/autoprefixer) (external)
+### [`autoprefixer`](shortcut://github.com/postcss/autoprefixer) (external)


Please use https

alexander-akait · 2020-01-28T10:56:49Z

packages/cssnano-preset-advanced/README.md

@@ -342,5 +342,5 @@ See [CONTRIBUTORS.md](https://github.com/cssnano/cssnano/blob/master/CONTRIBUTOR

 ## License

-MIT © [Ben Briggs](http://beneb.info)
+[MIT](https://opensource.org/licenses/MIT) © [Ben Briggs](http://beneb.info)


Do not change it

alexander-akait

Please remove changes that are not related to the issue, like

import processCss, { passthrough } from './_processCss';

->

import processCss from './_processCss';

It pollutes the git history, let's do it in a different PR

Siilwyn · 2020-01-28T13:24:46Z

Alright I tried to reduce the diff amount @evilebottnawi 👍

Removing the import processCss, { passthrough } from './_processCss'; like you want does make the lint fail though.

Siilwyn · 2020-02-08T14:28:30Z

@evilebottnawi ah yes didn't think of that. I have published a new version that uses postcss to strip the vendor prefix before comparing properties. Also updated the tests.

alexander-akait

Great!

Can we add tests for at-rules - @media, @supports, @font-face. For example:

/cc @ben-eb Can you look at this? Maybe i missed something?

Siilwyn · 2020-02-10T13:40:01Z

@evilebottnawi the sorter only runs within at-rules so PostCSS will handle the AST, what do you want to test?

Things like, for example the following?

test(
  'keep order of short- and longhand properties, within at-rule',
  passthroughCSS('@supports (display: grid) {div {animation: a; animation-name: b}}')
);

alexander-akait · 2020-02-10T13:42:17Z

@Siilwyn yes, also we should sort properties inside at-rules

alexander-akait · 2020-02-12T11:38:56Z

@Siilwyn please avoid using require('fs').promises for node@10, because it is create the warning and annoy developers, they will create a issue about it

alexander-akait · 2020-02-12T11:54:47Z

Also

const postcss = require('postcss');
const cssDeclarationSorter = require('css-declaration-sorter');

const code = /* CSS */ `
a {
  animation-name: Three;
  animation: Two 2s;
  -webkit-animation: One 2s;
  -webkit-name: Four;
}
`;

postcss([cssDeclarationSorter({ order: 'alphabetical', keepOverrides: true })])
  .process(code)
  .then((result) => console.log(result.css));

Still broken, output:

a {
  -webkit-name: Four;
  animation-name: Three;
  animation: Two 2s;
  -webkit-animation: One 2s;
}

-webkit-name should be after -webkit-animation

Siilwyn · 2020-02-12T12:09:18Z

What is -webkit-name? I can't find this property anywhere on google or MDN docs.

Siilwyn · 2020-02-12T12:15:14Z

The FS promises API is stable and won't produce a warning with node@10, see the docs: https://nodejs.org/dist/latest-v10.x/docs/api/fs.html#fs_fs_promises_api

alexander-akait · 2020-02-12T12:17:06Z

@Siilwyn oh, my mistake, sorry, all work fine, i mean -webkit-animation-name 😄

alexander-akait · 2020-02-12T12:21:57Z

@Siilwyn it is stable for LTS release, but for non LTS releases it outputs the warning, for example use v10.15.2 and you will see the warning

alexander-akait · 2020-02-12T12:22:22Z

Also please fix conflicts

alexander-akait · 2020-02-12T12:25:43Z

Also we have interesting case:

const postcss = require('postcss');
const cssDeclarationSorter = require('css-declaration-sorter');

const code = /* CSS */ `
a {
  animation-name: Three;
  animation: Two 2s;
  animation-future-new-long-property: 20s;
}
`;

postcss([cssDeclarationSorter({ order: 'alphabetical', keepOverrides: true })])
  .process(code)
  .then((result) => console.log(result.css));

Output:

a {
  animation-future-new-short-property: 20px;
  animation-name: Three;
  animation: Two 2s;
}

But will be great to keep that property in current place:

a {
  animation-name: Three;
  animation: Two 2s;
  animation-future-new-short-property: 20px;
}

In theory, in future any short property can have a new long property, so we should keep them in right place, what do you think?

I known it is the rare case, but for the full safe mode, we should consider this

alexander-akait · 2020-02-18T16:25:55Z

/cc @Siilwyn friendly ping

Siilwyn · 2020-02-18T18:49:20Z

@evilebottnawi I can keep unkown properties in place but it does 'block' the surrounding properties from being sorted.
E.g this will stay like this after sorting:
a{z-index: 0; unkown-a: 0; border: 0;}

Update: this looks hard to do actually since properties not direct to an unkown property will 'jump' over:
a{z-index: 0;color: 0;unkown-a: 0;border: 0;} > a{border: 0;color: 0;z-index: 0;unkown-a: 0;}

Since CSS properties first go through a draft stage before getting accepted and such would you be okay with not handling future unknown properties? I'm keeping the properties up-to-date by scraping from MDN.

alexander-akait · 2020-02-19T10:52:36Z

@Siilwyn Yes, new unknown properties is very rare edge case, let's finish this PR:

avoid using fs.promise to avoid warning on non LTS
added a new test - `:root {--a-var: 'value'; --c-var: calc(10px + 20px); --b-var: 12px; }
fix conflicts

Siilwyn · 2020-02-19T23:44:05Z

@evilebottnawi
✔️
✔️
✔️

All done! :)

alexander-akait · 2020-02-20T09:40:54Z

@Siilwyn Good job!

To be honesty we can change order of variables too :root {--a-var: 'value'; --c-var: calc(10px + 20px); --b-var: 12px; }.

For example:

:root {
 --b: var(--a);
 --a: 10px;
}

It is work.

But it is feature. Let's open issue in css-declaration-sorter for that?

alexander-akait · 2020-02-20T09:42:49Z

Already exists Siilwyn/css-declaration-sorter#54 😄

Siilwyn · 2020-02-20T09:47:52Z

Haha yes, but should not be needed for this PR right? Anything I need to change?

alexander-akait · 2020-02-20T10:54:45Z

Yes, we can solve this late, not high priority

Siilwyn · 2020-02-20T11:11:07Z

Yes! 🎉

alexander-akait reviewed Dec 12, 2019

View reviewed changes

packages/cssnano-preset-default/package.json Outdated Show resolved Hide resolved

alexander-akait reviewed Dec 17, 2019

View reviewed changes

Siilwyn added 12 commits January 11, 2020 14:27

Version upgrade an lock files update

292832a

Remove all css-declaration-sorter passthrough tests

88a89b6

Update snapshots

15b7619

Enable keep overrides option

eab8b74

Fix merging test

5caa495

Run the 'build:integration' script

bfdaf2f

Remove unused test help function

5d58c6a

Change to new version tag 5.0

5d9b902

Reset lockfiles

959afee

Update css-declarations-sorter to new version

a6c3739

Add default preset integration tests

20994a2

Run fixlint

e8ca00e

Siilwyn added 2 commits January 24, 2020 10:58

Merge branch 'master' into upgrade-css-declaration-sorter

48165a5

Rebuild integration tests

a022e02

alexander-akait reviewed Jan 28, 2020

View reviewed changes

Siilwyn added 2 commits January 28, 2020 14:22

Reset script changes

e5bc586

Add back unused passthrough test function

4919508

Handle vendor prefixed properties

592fc7a

Increase timeout for mac integration tests

47b11d4

alexander-akait reviewed Feb 10, 2020

View reviewed changes

Add at-rule tests

92c4bf4

Siilwyn added 4 commits February 20, 2020 00:12

Add test for CSS variables order

20cd9a7

Merge branch 'master' into upgrade-css-declaration-sorter

217a387

Update so now fs warnings show up

60d46f6

Fix linting errors

cd1a871

alexander-akait merged commit 613d562 into cssnano:master Feb 20, 2020

alexander-akait mentioned this pull request Feb 20, 2020

cssDeclarationSorter is not safe when shorthand after longhand property #535

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade css declaration sorter #855

Upgrade css declaration sorter #855

Siilwyn commented Dec 12, 2019

codecov-io commented Dec 12, 2019 •

edited

alexander-akait left a comment

Siilwyn commented Dec 17, 2019

Siilwyn commented Jan 3, 2020

Siilwyn commented Jan 11, 2020

ben-eb commented Jan 13, 2020

Siilwyn commented Jan 14, 2020

ben-eb commented Jan 14, 2020

Siilwyn commented Jan 28, 2020

alexander-akait Jan 28, 2020

alexander-akait Jan 28, 2020

alexander-akait left a comment

Siilwyn commented Jan 28, 2020 •

edited

Siilwyn commented Feb 8, 2020

alexander-akait left a comment

Siilwyn commented Feb 10, 2020

alexander-akait commented Feb 10, 2020

alexander-akait commented Feb 12, 2020

alexander-akait commented Feb 12, 2020

Siilwyn commented Feb 12, 2020

Siilwyn commented Feb 12, 2020

alexander-akait commented Feb 12, 2020 •

edited

alexander-akait commented Feb 12, 2020

alexander-akait commented Feb 12, 2020

alexander-akait commented Feb 12, 2020 •

edited

alexander-akait commented Feb 18, 2020

Siilwyn commented Feb 18, 2020 •

edited

alexander-akait commented Feb 19, 2020 •

edited

Siilwyn commented Feb 19, 2020

alexander-akait commented Feb 20, 2020

alexander-akait commented Feb 20, 2020

Siilwyn commented Feb 20, 2020 •

edited

alexander-akait commented Feb 20, 2020

Siilwyn commented Feb 20, 2020

Upgrade css declaration sorter #855

Upgrade css declaration sorter #855

Conversation

Siilwyn commented Dec 12, 2019

codecov-io commented Dec 12, 2019 • edited

Codecov Report

alexander-akait left a comment

Choose a reason for hiding this comment

Siilwyn commented Dec 17, 2019

Siilwyn commented Jan 3, 2020

Siilwyn commented Jan 11, 2020

ben-eb commented Jan 13, 2020

Siilwyn commented Jan 14, 2020

ben-eb commented Jan 14, 2020

Siilwyn commented Jan 28, 2020

alexander-akait Jan 28, 2020

Choose a reason for hiding this comment

alexander-akait Jan 28, 2020

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

Siilwyn commented Jan 28, 2020 • edited

Siilwyn commented Feb 8, 2020

alexander-akait left a comment

Choose a reason for hiding this comment

Siilwyn commented Feb 10, 2020

alexander-akait commented Feb 10, 2020

alexander-akait commented Feb 12, 2020

alexander-akait commented Feb 12, 2020

Siilwyn commented Feb 12, 2020

Siilwyn commented Feb 12, 2020

alexander-akait commented Feb 12, 2020 • edited

alexander-akait commented Feb 12, 2020

alexander-akait commented Feb 12, 2020

alexander-akait commented Feb 12, 2020 • edited

alexander-akait commented Feb 18, 2020

Siilwyn commented Feb 18, 2020 • edited

alexander-akait commented Feb 19, 2020 • edited

Siilwyn commented Feb 19, 2020

alexander-akait commented Feb 20, 2020

alexander-akait commented Feb 20, 2020

Siilwyn commented Feb 20, 2020 • edited

alexander-akait commented Feb 20, 2020

Siilwyn commented Feb 20, 2020

codecov-io commented Dec 12, 2019 •

edited

Siilwyn commented Jan 28, 2020 •

edited

alexander-akait commented Feb 12, 2020 •

edited

alexander-akait commented Feb 12, 2020 •

edited

Siilwyn commented Feb 18, 2020 •

edited

alexander-akait commented Feb 19, 2020 •

edited

Siilwyn commented Feb 20, 2020 •

edited