svgo icon indicating copy to clipboard operation
svgo copied to clipboard

Remove `stable` package in favor of native stable sort

Open boidolr opened this issue 3 years ago • 15 comments

Currently when installing svgo the following warning is displayed:

npm WARN deprecated [email protected]: Modern JS already guarantees Array#sort() is a stable sort, so this library is deprecated. See the compatibility table on MDN: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort#browser_compatibility

This PR removes that warning by removing stable. As the stable package notice says Array.sort(..) guarantees a stable sort for reasonable recent versions of JavaScript already. See caniuse data to decide whether to accept this PR.

boidolr avatar Jun 24 '22 09:06 boidolr

@TrySound can we have this merged. NPM Warn is not good.

gian-luca-weston avatar Jun 24 '22 12:06 gian-luca-weston

See also https://github.com/svg/svgo/issues/1680, https://github.com/svg/svgo/issues/1689 and notably https://github.com/svg/svgo/issues/1429 as it implies a node >=12

boidolr avatar Jul 06 '22 14:07 boidolr

+1 on merge, please :)

arderyp avatar Aug 24 '22 19:08 arderyp

我支持这门婚事

liubaicai avatar Aug 26 '22 04:08 liubaicai

@boidolr Do you want to release this fork? I can use it using pnpm overrides in the meantime.

https://pnpm.io/package_json#pnpmoverrides

aminya avatar Aug 28 '22 00:08 aminya

@aminya sorry I do not plan to maintain a package. You are more than welcome to use these changes :)

boidolr avatar Sep 03 '22 08:09 boidolr

@TrySound can we have this merged. NPM Warn is annoying.

kingyue737 avatar Sep 04 '22 10:09 kingyue737

I used the patch file below to apply this PR using patch-package, and manually edited package-lock.json to remove the dependency. Works like a charm! Thanks @boidolr 🙏

patches/svgo+2.8.0.patch
Patch svgo to remove dependency on deprecated stable package.
from https://github.com/svg/svgo/pull/1681/commits/f97238e61dce48822b7faa67173acce08f51c176

diff --git a/node_modules/svgo/lib/css-tools.js b/node_modules/svgo/lib/css-tools.js
index a59aae6..f14fba8 100644
--- a/node_modules/svgo/lib/css-tools.js
+++ b/node_modules/svgo/lib/css-tools.js
@@ -2,7 +2,6 @@

 var csstree = require('css-tree'),
   List = csstree.List,
-  stable = require('stable'),
   specificity = require('csso/lib/restructure/prepare/specificity');

 /**
@@ -162,7 +161,7 @@ function _bySelectorSpecificity(selectorA, selectorB) {
  * @return {Array} Stable sorted selectors
  */
 function sortSelectors(selectors) {
-  return stable(selectors, _bySelectorSpecificity);
+  return [...selectors].sort(_bySelectorSpecificity);
 }

 /**
diff --git a/node_modules/svgo/lib/style.js b/node_modules/svgo/lib/style.js
index 1873e7b..d575631 100644
--- a/node_modules/svgo/lib/style.js
+++ b/node_modules/svgo/lib/style.js
@@ -13,7 +13,6 @@
  * @typedef {import('./types').XastChild} XastChild
  */

-const stable = require('stable');
 const csstree = require('css-tree');
 // @ts-ignore not defined in @types/csso
 const specificity = require('csso/lib/restructure/prepare/specificity');
@@ -249,9 +248,7 @@ const collectStylesheet = (root) => {
     },
   });
   // sort by selectors specificity
-  stable.inplace(rules, (a, b) =>
-    compareSpecificity(a.specificity, b.specificity)
-  );
+  rules.sort((a, b) => compareSpecificity(a.specificity, b.specificity));
   return { rules, parents };
 };
 exports.collectStylesheet = collectStylesheet;
diff --git a/node_modules/svgo/package.json b/node_modules/svgo/package.json
index d827b03..a97f8a3 100644
--- a/node_modules/svgo/package.json
+++ b/node_modules/svgo/package.json
@@ -105,8 +105,7 @@
     "css-select": "^4.1.3",
     "css-tree": "^1.1.3",
     "csso": "^4.2.0",
-    "picocolors": "^1.0.0",
-    "stable": "^0.1.8"
+    "picocolors": "^1.0.0"
   },
   "devDependencies": {
     "@rollup/plugin-commonjs": "^20.0.0",
diff --git a/node_modules/svgo/plugins/inlineStyles.js b/node_modules/svgo/plugins/inlineStyles.js
index a19f3fb..bdd5090 100644
--- a/node_modules/svgo/plugins/inlineStyles.js
+++ b/node_modules/svgo/plugins/inlineStyles.js
@@ -9,7 +9,6 @@
 const csstree = require('css-tree');
 // @ts-ignore not defined in @types/csso
 const specificity = require('csso/lib/restructure/prepare/specificity');
-const stable = require('stable');
 const {
   visitSkip,
   querySelectorAll,
@@ -200,11 +199,13 @@ exports.fn = (root, params) => {
           return;
         }
         // stable sort selectors
-        const sortedSelectors = stable(selectors, (a, b) => {
-          const aSpecificity = specificity(a.item.data);
-          const bSpecificity = specificity(b.item.data);
-          return compareSpecificity(aSpecificity, bSpecificity);
-        }).reverse();
+        const sortedSelectors = [...selectors]
+          .sort((a, b) => {
+            const aSpecificity = specificity(a.item.data);
+            const bSpecificity = specificity(b.item.data);
+            return compareSpecificity(aSpecificity, bSpecificity);
+          })
+          .reverse();

         for (const selector of sortedSelectors) {
           // match selectors

andyjy avatar Sep 06 '22 13:09 andyjy

This does appear to be a breaking change, as SVGO also supports Node 10, which doesn't include stable sorting.

RDIL avatar Sep 12 '22 03:09 RDIL

@RDIL One could say that this breaking change is acceptable due to the age (and lack of support) of Node 10.

scherii avatar Sep 12 '22 06:09 scherii

I used the patch file below to apply this PR using patch-package, and manually edited package-lock.json to remove the dependency.

@andyjy I got an error when applying the patch using your copy-pasted file content via pnpm. Not sure why.

I recommend others to create the patch directly from GitHub patches/svgo+2.8.0.patch https://github.com/svg/svgo/pull/1681.patch

(replace a/ and b/ with a/node_modules/svgo and b/node_modules/svgo)

aminya avatar Sep 14 '22 08:09 aminya

@TrySound can we have this merged. NPM Warn is annoying.

@TrySound

jd-solanki avatar Sep 16 '22 13:09 jd-solanki

For people who can't endure the warning. I publish this fork in npm https://www.npmjs.com/package/@kingyue/svgo.

pnpm users can override the original svgo with

// package.json
"pnpm": {
  "overrides": {
    "svgo": "npm:@kingyue/svgo@^2.9.0",
  }
}

kingyue737 avatar Sep 18 '22 09:09 kingyue737

That's kind but less than ideal. Is it possible this package has been abandoned?

arderyp avatar Sep 18 '22 13:09 arderyp

@deepsweet sorry for bothering you, I know you're not that active in this repo, but you are listed as an administrator of svgo, maybe you can review and hopefully merge this. yours

Cajuteq avatar Sep 20 '22 19:09 Cajuteq

@TrySound ping

HansBrende avatar Oct 01 '22 00:10 HansBrende

Pong

TrySound avatar Oct 01 '22 17:10 TrySound

Thanks @boidolr

TrySound avatar Oct 01 '22 18:10 TrySound