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

Replace chalk with picocolors #175

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

onigoetz
Copy link

@onigoetz onigoetz commented Jan 2, 2022

Hi,

I'm trying to reduce the size of the dependencies in my dependencies.

chalk is a quite big library, and picocolors is much smaller and more performant.

I tried hard to get the tests running locally and made sure the ones that can run are running fine, but I have these errors:

❯ yarn test
yarn run v1.17.3
$ builder concurrent --buffer test-bin test-lib

TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" for /Users/onigoetz/Sites/Libs/inspectpack/test/lib/actions/duplicates.spec.ts
    at Loader.defaultGetFormat [as _getFormat] (internal/modules/esm/get_format.js:65:15)
    at Loader.getFormat (internal/modules/esm/loader.js:116:42)
    at Loader.getModuleJob (internal/modules/esm/loader.js:247:31)
    at Loader.import (internal/modules/esm/loader.js:181:17)
    at Object.exports.loadFilesAsync (/Users/onigoetz/Sites/Libs/inspectpack/node_modules/mocha/lib/esm-utils.js:33:20)
    at singleRun (/Users/onigoetz/Sites/Libs/inspectpack/node_modules/mocha/lib/cli/run-helpers.js:125:3)
    at Object.exports.handler (/Users/onigoetz/Sites/Libs/inspectpack/node_modules/mocha/lib/cli/run.js:362:5)


  bin/inspectpack
    - needs more tests
    -h
      ✓ displays help by default with error exit (145ms)
      ✓ displays help with flag with normal exit (106ms)
    --bail
      ✓ passes on simple sizes (117ms)
      ✓ passes on simple duplicates (109ms)
      ✓ passes on simple versions (110ms)
      ✓ passes on duplicates-esm sizes (115ms)
      ✓ bails on duplicates-esm duplicates (113ms)
      ✓ bails on duplicates-esm versions (123ms)

  bin/lib/args
    - needs tests


  8 passing (947ms)
  2 pending

[builder:finish] Hit 2 errors: 
  * Error: Command failed: sh -c mocha "test/lib/**/*.spec.ts"

  * Error: Command failed: sh -c mocha "test/lib/**/*.spec.ts"

[builder:builder-core:end:19718] Task: concurrent test-bin, test-lib, Error: Command failed: sh -c mocha "test/lib/**/*.spec.ts"

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

On another topic, I see that fp-ts is really a big dependency too, I would like to make a PR to replace it with something else if you'd be interested in that ? I'm not sure exactly what fp-ts brings though.

@ryan-roemer
Copy link
Member

My suspicion is actually that the addition of strip-ansi is mucking things up. Our TS is compiled to CommonJS format.

The author of strip-ansi (well and chalk too 😛 ) has been aggressively moving over packages to ESM-only. strip-ansi only now works in ESM -- see, e.g. https://unpkg.com/browse/strip-ansi@7.0.1/package.json ("type": "module" means ESM-only).

You might want to try switching to https://unpkg.com/browse/strip-ansi@6.0.1/, the last CJS-compatible release.

@ryan-roemer
Copy link
Member

I checked things out and things are looking pretty good. The refactoring of some tests were actually failing because of comparing incorrect things. I downgraded to strip-ansi@6.0.1 which is still CJS, changed the imports and cleaned up some stuff.

Here's a diff that hopefully helps continue things along:

diff --git a/package.json b/package.json
index 0ccfc18..7635ea8 100644
--- a/package.json
+++ b/package.json
@@ -93,7 +93,7 @@
     "sinon": "^9.0.2",
     "sinon-chai": "^3.5.0",
     "source-map-support": "^0.5.19",
-    "strip-ansi": "^7.0.1",
+    "strip-ansi": "^6.0.1",
     "ts-node": "^9.1.1",
     "tslint": "^6.1.2",
     "typescript": "^4.1.3",
diff --git a/test/lib/actions/versions.spec.ts b/test/lib/actions/versions.spec.ts
index 271a629..4351749 100644
--- a/test/lib/actions/versions.spec.ts
+++ b/test/lib/actions/versions.spec.ts
@@ -1,7 +1,7 @@
 import { join, resolve, sep } from "path";
 
 import { expect } from "chai";
-import stripAnsi from "strip-ansi";
+import stripAnsi = require("strip-ansi");
 import * as merge from "deepmerge";
 import * as mock from "mock-fs";
 
diff --git a/test/lib/plugin/duplicates.spec.ts b/test/lib/plugin/duplicates.spec.ts
index a86d748..2f4fe19 100644
--- a/test/lib/plugin/duplicates.spec.ts
+++ b/test/lib/plugin/duplicates.spec.ts
@@ -15,7 +15,7 @@ import {
    ICompilation,
 } from "../../../src/plugin/duplicates";
 
-import stripAnsi from "strip-ansi";
+import stripAnsi = require("strip-ansi");
 import { IWebpackStats } from "../../../src/lib/interfaces/webpack-stats";
 import { toPosixPath } from "../../../src/lib/util/files";
 import { IFixtures, loadFixtures, VERSIONS } from "../../utils";
@@ -257,7 +257,7 @@ foo (Found 1 resolved, 2 installed, 2 depended. Latest 1.1.1.)
                 .to.have.lengthOf(1).and
                 .to.have.property("0").that
                 .is.an("Error");
-              expect(stripAnsi(compilation.warnings[0].message)).to.eql(defaultReport);
+              expect(stripAnsi(compilation.errors[0].message)).to.eql(defaultReport);
             });
           });
 
@@ -273,7 +273,7 @@ foo (Found 1 resolved, 2 installed, 2 depended. Latest 1.1.1.)
                 .to.have.lengthOf(1).and
                 .to.have.property("0").that
                 .is.an("Error");
-              expect(stripAnsi(compilation.warnings[0].message)).to.eql(verboseReport);
+              expect(stripAnsi(compilation.errors[0].message)).to.eql(verboseReport);
             });
           });
 
diff --git a/test/utils.ts b/test/utils.ts
index dff9859..29568d4 100644
--- a/test/utils.ts
+++ b/test/utils.ts
@@ -1,5 +1,5 @@
 import { basename, join, relative, sep } from "path";
-import stripAnsi from "strip-ansi";
+import stripAnsi = require("strip-ansi");
 
 import { IModule } from "../src/lib/interfaces/modules";
 import { IWebpackStats } from "../src/lib/interfaces/webpack-stats";
diff --git a/yarn.lock b/yarn.lock
index 66a4e18..34435cc 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -844,10 +844,10 @@ ansi-regex@^5.0.0:
   resolved "https://registry.yarnpkg.com/ansi-regex/-/ansi-regex-5.0.0.tgz#388539f55179bf39339c81af30a654d69f87cb75"
   integrity sha512-bY6fj56OUQ0hU1KjFNDQuJFezqKdrAyFdIevADiqrWHwSlbmBNMHp5ak2f40Pm8JTFyM2mqxkG6ngkHO11f/lg==
 
-ansi-regex@^6.0.1:
-  version "6.0.1"
-  resolved "https://registry.yarnpkg.com/ansi-regex/-/ansi-regex-6.0.1.tgz#3183e38fae9a65d7cb5e53945cd5897d0260a06a"
-  integrity sha512-n5M855fKb2SsfMIiFFoVrABHJC8QtHwVx+mHWP3QcEqBHYienj5dHSgjbxtC0WEZXYt4wcD6zrQElDPhFuZgfA==
+ansi-regex@^5.0.1:
+  version "5.0.1"
+  resolved "https://registry.yarnpkg.com/ansi-regex/-/ansi-regex-5.0.1.tgz#082cb2c89c9fe8659a311a53bd6a4dc5301db304"
+  integrity sha512-quJQXlTSUGL2LH9SUXo8VwsY4soanhgo6LNSm84E1LBcE8s3O0wpdiRzyR9z/ZZJMlMWv37qOOb9pdJlMUEKFQ==
 
 ansi-styles@^3.2.0, ansi-styles@^3.2.1:
   version "3.2.1"
@@ -5872,12 +5872,12 @@ strip-ansi@^6.0.0:
   dependencies:
     ansi-regex "^5.0.0"
 
-strip-ansi@^7.0.1:
-  version "7.0.1"
-  resolved "https://registry.yarnpkg.com/strip-ansi/-/strip-ansi-7.0.1.tgz#61740a08ce36b61e50e65653f07060d000975fb2"
-  integrity sha512-cXNxvT8dFNRVfhVME3JAe98mkXDYN2O1l7jmcwMnOslDeESg1rF/OZMtK0nRAhiari1unG5cD4jG3rapUAkLbw==
+strip-ansi@^6.0.1:
+  version "6.0.1"
+  resolved "https://registry.yarnpkg.com/strip-ansi/-/strip-ansi-6.0.1.tgz#9e26c63d30f53443e9489495b2105d37b67a85d9"
+  integrity sha512-Y38VPSHcqkFrCpFnQ9vuSXmquuv5oXOKpGeT6aGrr3o3Gc9AlVa6JBfUSOCnbxGGZF+/0ooI7KrPuUSztUdU5A==
   dependencies:
-    ansi-regex "^6.0.1"
+    ansi-regex "^5.0.1"
 
 strip-bom@^2.0.0:
   version "2.0.0"

@onigoetz
Copy link
Author

onigoetz commented Jan 4, 2022

Indeed by downgrading to strip-ansi 6.0.1 it's much better.
Now I have failing test related to node_modules, they seem to fail with a timeout. not sure why.
Will continue to investigate for now

However about your proposal to change

-              expect(stripAnsi(compilation.warnings[0].message)).to.eql(verboseReport);
+              expect(stripAnsi(compilation.errors[0].message)).to.eql(verboseReport);

I'm not sure that's correct as the property checked above is a warning and I kept it.

edit: oops my bad, it was indeed incorrect

@onigoetz
Copy link
Author

onigoetz commented Jan 4, 2022

I was able to get all tests to be green.

However it seems that src/lib/util/files.ts's readJson doesn't run on Node.js 16. It timeouts without a warning.

Sometimes I'm able to get the message "ENXIO: no such device or address, read" out of it. but I'm not sure why it doesn't bubble up the promise chain.

@onigoetz
Copy link
Author

Having a look at older PRs of mine, can we merge this?

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

Successfully merging this pull request may close these issues.

2 participants