lirantal / lockfile-lint

Lint an npm or yarn lockfile to analyze and detect security issues
Apache License 2.0
780 stars 35 forks source link

fix #125 - remove debug dependency from lockfile-lint-api #127

Closed naugtur closed 2 years ago

naugtur commented 2 years ago

Just getting rid of a dependency that, while barely used, pulls in a bunch of requirements. Here's an overview:

"lockfile-lint-api>debug": {
      "builtin": {
        "tty.isatty": true,
        "util.deprecate": true,
        "util.format": true,
        "util.inspect": true
      },
      "globals": {
        "console": true,
        "document": true,
        "localStorage": true,
        "navigator": true,
        "process": true
      },
      "packages": {
        "lavamoat>@babel/highlight>chalk>supports-color": true,
        "lockfile-lint-api>debug>ms": true
      }
    },

This is also contributing to the progress on #123 by getting rid of tty and process requirements

BREAKING:

bumped engine to v10 to remove all const {URL} = require('url')

I saw semantic-release is used, so this may need to be split into two conventional commits

naugtur commented 2 years ago

@lirantal Before I merge this, can you confirm it's ok to stop supporting node.js version 8? I think it's been dead long enough, but maybe there's reasons?

lirantal commented 2 years ago

A few observations here:

  1. Are we replacing debug only with lockfile-lint-api ?
  2. There are uses of debug here and packages/lockfile-lint/src/main.js too
  3. We should publish a new major version for any breaking changes like dropping 8.x (which I'm ok to do, just want to make sure we're not breaking any dependents)
naugtur commented 2 years ago
1. Are we replacing `debug` only with `lockfile-lint-api` ?

Yes, the CLI has a lot of other dependencies and for now I'm focusing on narrowing down lockfile-lint-api to a minimal scope of outside requirements to make it super easy to tightly secure with lavamoat policy or even run in a compartment with lockdown.

2. There are uses of debug [here](https://github.com/lirantal/lockfile-lint/blob/7aa0b3d635292718331e6d08f39a89265e6b5109/packages/lockfile-lint/src/validators/index.js#L21-L24) and [packages/lockfile-lint/src/main.js](https://github.com/lirantal/lockfile-lint/blob/7aa0b3d635292718331e6d08f39a89265e6b5109/packages/lockfile-lint/src/main.js) too

Yes, just api for now. Debug is a good thing and the package that uses TTY anyway does not need to be rid of it. I might pursue removing dependencies and builtin use from the CLI later, but that's outside the scope I was ready to tackle now.

3. We should publish a new major version for any breaking changes like dropping 8.x (which I'm ok to do, just want to make sure we're not breaking any dependents)

Yes, it'd be the second breaking change I merge, so the major version is definitely going up.
I just wanted to make sure there's not a known usecase for node 8.x support.

lirantal commented 2 years ago

All sounds good to me :-)