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 rimraf and mkdirp dependencies #32

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

davidrapin
Copy link

Problem to solve

  • rimram is installed as a prod dependency but is only used in tests in one place, it has a lot of sub-dependencies, some of them with security problems.
  • mkdirp is only used in one place

Solution

  • rimraf is not needed anymore, since Node.js has an equivalent function (rmSync with recursive=true) since Node.js v14.14 (source)
  • mkdirp is not needed anymore, since Node.js has an equivalent function (mkdirSync with recursive=true) since Node.js v10.12 (source)

@@ -4,17 +4,15 @@

import t from 'tap'
import { BrotliDecompress } from '../dist/esm/index.js'
import fs from 'fs'
import fs from 'node:fs'

Choose a reason for hiding this comment

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

it's a bit inconsistent that node: was added here, but not the two lines below

Copy link
Author

Choose a reason for hiding this comment

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

Indeed, open to change that, I wanted to keep this PR focused to increase the chances of it getting merged.

@43081j
Copy link

43081j commented Dec 2, 2024

@isaacs any chance we can get a review of this?

Looks like ci minimum version is high enough to support this too

@DaleGardner
Copy link

DaleGardner commented Jan 3, 2025

@isaacs This package currently has a high severity security vulnerability (because of an issue with a dependency of rimraf) until this PR is merged and released. Info on the vuln:

https://security.snyk.io/vuln/SNYK-JS-CROSSSPAWN-8303230
[email protected] > [email protected] > [email protected] > [email protected] > [email protected]

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.

4 participants