raszi / node-tmp

Temporary file and directory creator for node.js
MIT License
736 stars 92 forks source link

fix #155 #161

Closed silkentrance closed 5 years ago

silkentrance commented 6 years ago

With this in place, the user will be handed out an async remove callback, as requested in #155. Internally, we will be using the sync remove callback so that on process exit, everything is done synchronously.

This also fixes existing async tests, at last.

silkentrance commented 6 years ago

@raszi are you ok with adding the rimraf dependency and eliminating our own code? as otherwise, we would have to reimplement rimraf for async operation.

If so, I would then proceed and merge this to master ASAP, after the cleanup of course.

raszi commented 6 years ago

Well, I would have no problems introducing a small dependency for the job and removing our code. My concerns with rimraf are the followings:

silkentrance commented 6 years ago

@raszi Sorry for not coming back to you and working on this for quite a while now. I will see whether there is a different approach to that.

silkentrance commented 6 years ago

@raszi the glob package is also used by npm itself, and, according to npm stats, it has a whopping 15mil (not US billion) downloads. In fact, it is a well proven dependency.

As to the failing tests, since we eliminated node < 0.10.0 the tests are no longer failing.

silkentrance commented 6 years ago

@raszi I fixed the order by which the tests cleanup their remnant temporary files. Sometimes, especially with node 0.10.x this would fail and done() would not be called.

silkentrance commented 6 years ago

rebased to master

kaiserfro commented 6 years ago

Can this PR be merged?

silkentrance commented 6 years ago

@kaiserfro From my part yes, but @raszi had some reservations towards the introduction of the dependency towards the glob package. As for merging, I can do that, but publishing a new version on npm I cannot do.

silkentrance commented 6 years ago

@kaiserfro since this integrates the latest changes from master, you could use a github reference for your dependency, unless you do not want to do that. alternatively you could publish this to an instance of verdaccio that you run on your site or even nexus acting as an npm registry.

silkentrance commented 5 years ago

@raszi since I have not heard back from you for a long time and since people actually need this functionality, I will continue with merging this into master as soon as all tests have succeeded. At least the appveyor builds are not working again.

raszi commented 5 years ago

Sorry for the delay, this is a super busy period of my life.

silkentrance commented 5 years ago

@raszi I am currently also rather busy. Thanks for the review. I will try to fix these issues ASAP.