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

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

Merged
merged 1 commit into from
Jun 11, 2022

Conversation

naugtur
Copy link
Collaborator

@naugtur naugtur commented May 25, 2022

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 naugtur mentioned this pull request May 25, 2022
3 tasks
@naugtur naugtur force-pushed the fearless-cooperation-inject-debug branch from dc34bdc to 721b878 Compare May 25, 2022 13:15
…move 'url' dependency, bump engine to v10
@naugtur naugtur force-pushed the fearless-cooperation-inject-debug branch from 721b878 to d84296a Compare June 9, 2022 12:03
@naugtur naugtur requested a review from lirantal June 9, 2022 12:47
@naugtur
Copy link
Collaborator Author

naugtur commented Jun 9, 2022

@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
Copy link
Owner

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)

module.exports = class ValidateHost {
constructor ({packages} = {}) {
constructor ({packages, debug = noop} = {}) {
Copy link
Owner

Choose a reason for hiding this comment

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

noop here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

noop by default, debug if passed.

Copy link
Owner

Choose a reason for hiding this comment

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

Ok my bad. I missed that you have actually defined it here const noop = () => {} a few lines before :-)

@naugtur
Copy link
Collaborator Author

naugtur commented Jun 11, 2022

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
Copy link
Owner

All sounds good to me :-)

@naugtur naugtur merged commit b4a8143 into lirantal:master Jun 11, 2022
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.

2 participants