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

feat: make it possible to omit glob dependency #192

Closed
wants to merge 3 commits into from

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Jul 14, 2019

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

iansu commented Jul 15, 2019

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

@bcoe
Copy link
Contributor Author

bcoe commented Jul 15, 2019

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

iansu commented Jul 15, 2019

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

isaacs commented Aug 14, 2019

Landed and published on v2.7.0. Thanks!

@isaacs
Copy link
Owner

isaacs commented Aug 14, 2019

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

iansu commented Aug 14, 2019

@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.

None yet

3 participants