raszi / node-tmp

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

Promises #74

Closed mlucool closed 8 years ago

mlucool commented 8 years ago

Hi,

Can we add an option to use promises instead of callbacks given its incorporation into ES6?

Thanks, Marc

sukima commented 8 years ago

Using bluebird you can promisify this like so:

var Promise = require('bluebird');
var tmp = require('node-tmp');

exports.file = function(opts) {
  var cleanupCallback;
  return new Promise(function(resolve, reject) {
    tmp.file(opts || {}, function(err, path, fd, callback) {
      cleanupCallback = callback;
      if (err) { return reject(err); }
      resolve([path, fd]);
    });
  })
  .finally(function() {
    cleanupCallback();
    cleanupCallback = null;
  });
};

exports.dir = function(opts) {
  var cleanupCallback;
  return new Promise(function(resolve, reject) {
    tmp.dir(opts || {}, function(err, path, callback) {
      cleanupCallback = callback;
      if (err) { return reject(err); }
      resolve(path);
    });
  })
  .finally(function() {
    cleanupCallback();
    cleanupCallback = null;
  });
};

You can also do this with native promises but that means you have to write your own then/catch functions.

mlucool commented 8 years ago

Yes I agree that is a workaround, but it would be better if it were baked in as a first call solution.

sukima commented 8 years ago

I would argue that making node-tmp depend on a Promise library is out of scope of this module. I think it would be better to make a node-tmp-promise module that wraps this one. In fact I might just do that. :thought_balloon:

sukima commented 8 years ago

I just attempted to do this on my own. I'm not sure there is a way. With a promise there isn't a way to prevent the cleanupCallback for happening after the promise is returned. Even with Bluebird's disposer I couldn't do it because it assumed that all the code that needs the temporary resource is inside the callback.

I can go into greater detail but suffice it to say I could wrap my head around decoupling the cleanupCallback.

benjamingr commented 8 years ago

Yeah OK I can do it.

benjamingr commented 8 years ago

Done at https://github.com/benjamingr/tmp-promise and https://www.npmjs.com/package/tmp-promise

Can PR into this repo as require("tmp").promised or something like that.

sukima commented 8 years ago

Wow, I'm impressed. I couldn't figure out how to clean up. And I'm impressed you were able to make cleanup() a global.

Fascinating

benjamingr commented 8 years ago

Yeah that's a mistake in the readme, it should be o.cleanup :D

You can use a disposer and avoid it altogether though.

deltaidea commented 8 years ago

Any news on this? What's the next step?

benjamingr commented 8 years ago

https://github.com/benjamingr/tmp-promise

silkentrance commented 8 years ago

@mlucool @sukima does tmp-promise solve your existing problems?

@benjamingr Thank you very much!

sukima commented 8 years ago

@silkentrance :+1: for tmp-promise package

silkentrance commented 8 years ago

I think that we can close this now as tmp-promise does what people expect.

mlucool commented 8 years ago

I have yet to play with tmp-promise, but any reason we can't merge this back into this project if we think it works well? One less dependency to worry about.

arcanis commented 7 years ago

Fwiw, from my experience, libraries such as tmp-promise are almost never really used, because they end up out-of-sync with the rest of the project, and require trusting projects that have much less scrutiny.

benjamingr commented 7 years ago

@arcanis last I checked tmp-promise had a surprising amount of usage (tens of thousands of downloads) which is not what I expected when I wrote it.

In the (hopefully near) future we'll reach consensus at NodeJS and we'll have tmp with promises built in.

rarkins commented 7 years ago

@benjamingr any update on whether to integrate promises into this package?

benjamingr commented 7 years ago

@rarkins yes, you can now util.promisify any Node API to get promises working there, and there is ongoing API work to eventually enable this.

silkentrance commented 7 years ago

@rarkins Please note that @benjamingr implemented some additional code in order to enable promises with tmp. Simply using util.promisify might not work. I recommend using tmp-promise.

ehmicky commented 5 years ago

Is there any plan to merge tmp-promise into node-tmp, so users don't have to either use tmp-promise or util.promisify to use promises with node-tmp?