Skip to content

feat: make it possible to omit glob dependency - #192

Closed
bcoe wants to merge 3 commits into
isaacs:masterfrom
bcoe:optional-glob
Closed

bcoe wants to merge 3 commits into
isaacs:masterfrom
bcoe:optional-glob

Conversation

@bcoe

@bcoe bcoe commented Jul 14, 2019

Copy link
Copy Markdown

makes it possible to omit glob dependency, so that we can more easily vendor rimraf in Node.js (see nodejs/node#28208).

This approach has been tested in Node.js, rimraf simply needs to be vendored in deps/rimraf and added to node.gyp.

CC: @iansu

@iansu

iansu commented Jul 15, 2019

Copy link
Copy Markdown

I've got things ready to go on the Node.js side once this is merged.

@bcoe

bcoe commented Jul 15, 2019

Copy link
Copy Markdown
Author

@iansu a couple thoughts for you:

  1. perhaps rename your PR on Node.js to something along the lines of fs: add fs.rmtree, and then in the description of your PR, point out that you're vendoring rimraf based on discussions with Myles.
  2. with regards to tests for rmtree, we should probably make an effort to test all the same cases demonstrated in test/basic.js.
  3. I'm excited about this; I think vendoring rimraf is a good start ... over time, I'd advocate we potentially solve more of rimraf's use-case at a lower level., e.g., in libuv.

@iansu

iansu commented Jul 15, 2019

Copy link
Copy Markdown

@bcoe as soon as this is merged I'll submit my updated PR and update the title and description. Then I'll start working on adding additional testcases. test/basic.js does look like a good baseline.

@isaacs isaacs closed this in dc1682d Aug 14, 2019
@isaacs

isaacs commented Aug 14, 2019

Copy link
Copy Markdown
Owner

Landed and published on v2.7.0. Thanks!

@isaacs

isaacs commented Aug 14, 2019

Copy link
Copy Markdown
Owner

@iansu Given that the tests in this module are rather shoddy, I wouldn't recommend using them as anything but a starting point. Patches welcome to add test coverage :)

@iansu

iansu commented Aug 14, 2019

Copy link
Copy Markdown

@isaacs Thanks for releasing this! I'll see if there are any test improvements we can contribute back.

@bcoe
bcoe deleted the optional-glob branch August 14, 2019 06:27
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