raszi / node-tmp

Temporary file and directory creator for node.js
MIT License
737 stars 93 forks source link

cleanup should accept a callback for composition #57

Closed bmeck closed 8 years ago

bmeck commented 9 years ago

right now cleanup is a synchronous function which cannot wrap a callback, this means you need to wrap any node convention callbacks like so:

// fs.writeFile...
function onFileWrite(err) {
  cleanup();
  return cb.apply(this, arguments);
}

or just a simple way to avoid having this liter code.

silkentrance commented 9 years ago

@bmeck please be more specific, to which cleanup() function do you refer to? There is none in the tmp API...

Do you mean the callback provided for removal of the temporary?

Why would you want to increase on the stack trace by indirecting a call to your "business" logic via the removal callback?

silkentrance commented 9 years ago

@raszi is this in any way related to node-tmp?

bmeck commented 9 years ago

@silkentrance I am referring to https://github.com/raszi/node-tmp/blob/216a2bf4544498ec42a43e718bee3e7b173ccb0a/lib/tmp.js#L387-L397 ,

right now any tool like aysnc off npm has to wrap the cleanup callback like above it if is a step in a series:

tmp.dir(function _tempDirCreated(err, path, cleanupCallback) {
// work with tmp file

// cleanup
async.series([
  ...
  function cleanupTmpFile(next) {
    cleanupCallback();
    next(null);
  },
  ...
], ...)
})

vs:

tmp.dir(function _tempDirCreated(err, path, cleanupCallback) {
// work with tmp file

// cleanup
async.series([
  ...
  cleanupCallback,
  ...
], ...)
})

this convention of calling another function is pretty prevalent in various APIs for node, but is not there for node-tmp

silkentrance commented 9 years ago

@bmeck Well, this makes it rather clear, thank you for the detailed information. The fix is rather simple and I am sure that you can provide @raszi with a PR for this as well.

silkentrance commented 8 years ago

@bmeck how about a PR or did the synchronous API solve your problems?