padolsey / operative

:dog2: Seamlessly create Web Workers
MIT License
655 stars 45 forks source link

Callbacks should have a node-style `(error, value)` interface #5

Closed Raynos closed 11 years ago

Raynos commented 11 years ago

This gives you interop with every piece of asynchronous code build around callbacks.

padolsey commented 11 years ago

Yet another pull request from Raynos! :) Thank you!

I can see the point in using the node-style callback, but I have two concerns:

  1. There doesn't seem to be a way to throw an error from within the worker and have it go through as callback(ERROR). Your mod will only ever do callback(null, result). There should probably be a way to throw an error in the worker and have it come through as a callback(theThrownError), right?
  2. Wondering how this reconciles with the pending support for promises. See #3.

ps: same tabbing issue with this diff. Can we use tabs only please?

Raynos commented 11 years ago

@padolsey workers should be able to error somehow but that's a seperate thing.

You can ignore the promises thing, callbacks are simpler and less opinionated then promises.

Also sorry I copy pasted your code into my editor to fix it up and converted everything to spaces >_<

Raynos commented 11 years ago

Converted back to tabs

padolsey commented 11 years ago

FWIW, I can see the appeal of promises. If there's a way for us to have both, I think that'd be best. The callback style would still be the default (i.e. a direct return from an operative method will go straight to your callback)

Raynos commented 11 years ago
var calculator = operative({
    add: function(a, b) {
        return a + b;
    }
});
var result = calculator.add.bind(null, 2, 3)

You don't need promises. Thunks or continuables are far more suited to the task.

padolsey commented 11 years ago

The appeal with promises is that I can explicitly fail (via reject()) without relying on throwing exceptions. Throwing exceptions becomes a real problem in asynchronous code, since we can't catch on a per-method basis -- it'll have to be caught on the entire worker... So basically, cb(AnActualError, null) can only occur in synchronous worker code.

Right now, the cb(err, res) style doesn't offer much if there's no formal way to trigger an actual error from within the worker.

Raynos commented 11 years ago

@padolsey return new Error("foo") explicit failure without throwing. I've done the same here ( https://github.com/Raynos/gens/blob/master/index.js#L25 )

padolsey commented 11 years ago

@Raynos That'll only work for synchronous exceptions though. If I'm doing, for example, a XHR request or some other async action (within an operative method) and it fails there's still no way for me to tell operative that I've failed.

Raynos commented 11 years ago

@padolsey operative is a sync to async thing. you can't really do async things in it.

If you want to do something async then it's trivial

var calculator = operative({
    add: function(a, b, cb) {
        cb(null, a + b);
    }
});
var result = calculator.add.bind(null, 2, 3)
calvinmetcalf commented 11 years ago

worker.addEventListener('error',function(){}) or worker.onerror = function(){} will grab any error that happens in a worker.

padolsey commented 11 years ago

I'm going to close this as Operative supports this pattern now:

var add = operative(function(a, b, callback) {
  if (typeof a != 'number' || typeof b != 'number') {
    callback('Arguments must be numbers');
    return;
  }
  callback(null, a + b);
});

add(111, 222, function(err, result) {
  console.log(arguments);
});