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

dep: remove mkdirp dependency #2084

Closed
wants to merge 2 commits into from

Conversation

rharriso
Copy link

#1071 ### Checklist

  • npm install && npm test passes
  • tests are included
  • documentation is changed or added
  • commit message follows commit guidelines
Description of change

mkdirp hasn't been updated in 4 years, and node's fs library
has had a recursive option for fs since v10.12 (oldest active LTS
version)

I was notified of an issue with minmist (a dependency of mkdirp) by npm audit. https://npmjs.com/advisories/1179. This library shouldn't be necessary as the old LTS supports recursive directory creation

cclauss and others added 2 commits March 19, 2020 02:15
* dep: mkdirp hasn't been updated in 4 years, and node's fs library
has had a recursive option for `fs` since v10.12 (oldest active LTS
version). Additionally `mkdirp` depends on a version of library `minimist` that has a minor vulnerability associated.

Refs:
* https://nodejs.org/api/fs.html#fs_fs_mkdir_path_options_callback
* https://npmjs.com/advisories/1179
Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

Yeah, this is fine. We'd proposed to do this for 6.x (or more likely 7.x) but we're going to have to stick with mkdirp for the remainder of 5.x's life and this is still the version that npm is choosing to pull in.
Just remove nodeFs and use fs directly and this should be good to go.

@@ -1,10 +1,10 @@
'use strict'

const nodeFs = require('fs')
Copy link
Member

Choose a reason for hiding this comment

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

remove this and just use fs, it should pass through properly

rvagg added a commit that referenced this pull request May 13, 2020
only supported on Node.js 10+

Closes: #2084
rvagg added a commit that referenced this pull request May 13, 2020
only supported on Node.js 10+

Closes: #2084
rvagg added a commit that referenced this pull request Jun 2, 2020
only supported on Node.js 10+

Closes: #2084
rvagg added a commit that referenced this pull request Jun 3, 2020
only supported on Node.js 10+

Closes: #2084
PR-URL: #2123
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@rvagg rvagg closed this in 4937722 Jun 3, 2020
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.

3 participants