FormidableLabs / inspectpack

An inspection tool for Webpack frontend JavaScript bundles.
MIT License
591 stars 20 forks source link

Replace chalk with picocolors #175

Open onigoetz opened 2 years ago

onigoetz commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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