ForbesLindesay / browserify-middleware

express middleware for browserify, done right
http://browserify.org
MIT License
381 stars 66 forks source link

Async support for hooks #60

Closed killroy42 closed 9 years ago

killroy42 commented 9 years ago

Hi, I propose this update to the hooks system to support async style hooks. I've made it backwards compatible to sync hooks by checking if the return value is a string. There are perhaps slightly more optimal ways of doing this, but this passes all the tests.

I used this to integrate closure compiler with the postcompile hook.

ForbesLindesay commented 9 years ago

I'm happy to accept the feature, but would like to use Promises instead of callbacks (although I don't mind adding support for callbacks as a fallback), and I refuse to use async ever, in any of my modules.

I would suggest we make safeHook look something like:

var Promise = require('promise');

function safeHook(hook) {
  if ('function' !== typeof hook) return function (src) { return Promise.resolve(src); };
  return function (src) {
    return new Promise(function (resolve, reject) {
      var src = hook(src, function (err, src) {
        if (err) reject(err);
        else resolve(src);
      });
      if (src) resolve(src);
    });
  }
}

This way, the result is always a promise for a string, and all hooks are always effectively asynchronous (including error handling).

Since we are now doing caching outside of the compile file, it would also be reasonable to have the entire overall compile api return a promise.

killroy42 commented 9 years ago

Agree on the async. I prefer callbacks on broad, generic libraries as they avoid surprises and avoid further dependencies. But if we can make it dual-purpose that would be best of course. Since starting to work in a node/browser mixed environment I've gotten really paranoid about dependencies!

ForbesLindesay commented 9 years ago

Use browserify, then stop being paranoid. As for surprising, outside of node core, promises have won this contest. Anyway, happy to have a callback fallback.

killroy42 commented 9 years ago

Well, for example uuid pulls in 60k of dependencies (including binary buffers, etc) and a simple version of it can be had for a couple of bytes. All of that matters in the browser, of course.

ForbesLindesay commented 9 years ago

Pulling in buffer is pretty expensive, pretty much anything else is usually cheap though.

killroy42 commented 9 years ago

I'm still learning. Been doing only backend for a year, and used to do frontend in the days of tiny-as-possible. So this is a happy meeting!

ForbesLindesay commented 9 years ago

This has now been added, just return a promise from your hook.

killroy42 commented 9 years ago

Thanks. Will be sure to test this out as I am about ready to work more on the deployment system.