Skip to content
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

Remove stable package in favor of native stable sort #1681

Merged
merged 1 commit into from
Oct 1, 2022
Merged

Remove stable package in favor of native stable sort #1681

merged 1 commit into from
Oct 1, 2022

Conversation

boidolr
Copy link
Contributor

@boidolr boidolr commented Jun 24, 2022

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.

@gian-luca-weston
Copy link

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

@boidolr
Copy link
Contributor Author

boidolr commented Jul 6, 2022

See also #1680, #1689 and notably #1429 as it implies a node >=12

Copy link

@scherii scherii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I applied this PR to an existing project without any issues.

LGTM! 🚀

@arderyp
Copy link

arderyp commented Aug 24, 2022

+1 on merge, please :)

@liubaicai
Copy link

我支持这门婚事

@aminya
Copy link

aminya commented Aug 28, 2022

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

https://pnpm.io/package_json#pnpmoverrides

@boidolr
Copy link
Contributor Author

boidolr commented Sep 3, 2022

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

@kingyue737
Copy link

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

@andyjy
Copy link

andyjy commented Sep 6, 2022

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

@RDIL
Copy link

RDIL commented Sep 12, 2022

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

@scherii
Copy link

scherii commented Sep 12, 2022

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

@aminya
Copy link

aminya commented Sep 14, 2022

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)

@jd-solanki
Copy link

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

@TrySound

@kingyue737
Copy link

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",
  }
}

@arderyp
Copy link

arderyp commented Sep 18, 2022

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

@Cajuteq
Copy link

Cajuteq commented Sep 20, 2022

@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

Copy link

@elmarbeckmann elmarbeckmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally and all working correctly. I can't do more than just approve here.

@HansBrende
Copy link

@TrySound ping

@TrySound
Copy link
Member

TrySound commented Oct 1, 2022

Pong

@TrySound
Copy link
Member

TrySound commented Oct 1, 2022

Thanks @boidolr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.