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

Use fs.rm() instead of rimraf #296

Merged
merged 1 commit into from Feb 29, 2024
Merged

Use fs.rm() instead of rimraf #296

merged 1 commit into from Feb 29, 2024

Conversation

kevinoid
Copy link
Contributor

A regression occurred in 0.2.2 which can be reproduced using:

const assert = require('assert');
const tmp = require('tmp');
tmp.dir({ unsafeCleanup: true }, (err, path, cleanup) => {
  assert.ifError(err);
  cleanup(assert.ifError);
});

This works with 0.2.1. With 0.2.2 it fails with:

/path/to/tmp/node-tmp/lib/tmp.js:358
        return removeFunction(fileOrDirName, next || function() {});
               ^

TypeError: removeFunction is not a function
    at _cleanupCallback (/path/to/tmp/node-tmp/lib/tmp.js:358:16)
    at /path/to/tmp/node-tmp/repro.js:5:3
    at _dirCreated (/path/to/tmp/node-tmp/lib/tmp.js:207:7)
    at FSReqCallback.oncomplete (node:fs:192:23)

This occurs because 00bb5b2 upgraded the rimraf dependency from ^3.0.0 to ^5.0.5 without handling the change to a Promise-based API in 4.0.0 (isaacs/rimraf@a71e7f9) or the removal of the default export in 5.0.0 (isaacs/rimraf@c7a3fd4).

This commit fixes the issue by dropping the rimraf dependency in favor of fs.rm({recursive: true}).

Thanks for considering,
Kevin

Fixes: #295

raszi
raszi previously approved these changes Feb 28, 2024
Copy link
Owner

@raszi raszi left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you very much!

@raszi
Copy link
Owner

raszi commented Feb 28, 2024

I believe the tests need to be updated as well.

@kevinoid
Copy link
Contributor Author

My pleasure. Thanks for reviewing it @raszi.

I believe the tests need to be updated as well.

Whoops! Right you are. Sorry I missed that.

I've pushed a new version which updates the tests, package-lock.json, and the docs. Let me know if I've missed anything else.

P.S. How do you feel about support for Node.js 14.0 to 14.14 (when fs.rm() was added)? I have a fallback from fs.rm() to fs.rmdir() in lib/tmp.js to support <14.14, but didn't bother in the tests, since CI doesn't run it. Would it make sense to drop support for Node.js < 14.14, or to test it and add a CI job?

@raszi
Copy link
Owner

raszi commented Feb 29, 2024

I don't mind dropping that it isn't supported anymore:

https://nodejs.org/en/about/previous-releases

image

@kevinoid
Copy link
Contributor Author

Sounds good to me. I've dropped the fs.rmdir fallback to simplify the code a bit. Let me know if there are any other changes you'd like.

@kevinoid
Copy link
Contributor Author

Note: I just pushed an update to generate package-lock.json with npm from Node 14 to fix the CI error.

package.json Outdated Show resolved Hide resolved
A regression occurred in 0.2.2 which can be reproduced using:

```js
const assert = require('assert');
const tmp = require('tmp');
tmp.dir({ unsafeCleanup: true }, (err, path, cleanup) => {
  assert.ifError(err);
  cleanup(assert.ifError);
});
```
This works with 0.2.1.  With 0.2.2 it fails with:

    /path/to/tmp/node-tmp/lib/tmp.js:358
            return removeFunction(fileOrDirName, next || function() {});
                   ^

    TypeError: removeFunction is not a function
        at _cleanupCallback (/path/to/tmp/node-tmp/lib/tmp.js:358:16)
        at /path/to/tmp/node-tmp/repro.js:5:3
        at _dirCreated (/path/to/tmp/node-tmp/lib/tmp.js:207:7)
        at FSReqCallback.oncomplete (node:fs:192:23)

This occurs because 00bb5b2 upgraded
the rimraf dependency from ^3.0.0 to ^5.0.5 without handling the change
to a `Promise`-based API in 4.0.0
(isaacs/rimraf@a71e7f9)
or the removal of the default export in 5.0.0
(isaacs/rimraf@c7a3fd4).

This commit fixes the issue by dropping the `rimraf` dependency in favor
of `fs.rm({recursive: true})`.

It also updates the nodejs engine version to 14.14, when `fs.rm()` was
added.

Fixes: 00bb5b2 ("Update rimraf and drop old Node compatibility")
Fixes: raszi#295
Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
Copy link
Owner

@raszi raszi left a comment

Choose a reason for hiding this comment

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

Nice work! Thank you very much!

@raszi raszi merged commit 6ed89d5 into raszi:master Feb 29, 2024
8 checks passed
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.

Upgrade of rimraf causes issue
2 participants