padolsey / operative

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

Worker methods to return promises #3

Closed darsain closed 11 years ago

darsain commented 11 years ago

Best would be probably to feature detect the native promises spec, and if present, return them from methods. People can than choose themselves to include polyfills.

A lot better than inlining it into the code, or adding a dependency...

rudylattae commented 11 years ago

+1

evanworley commented 11 years ago

+1

(BTW, love this project. Just suffered through doing native web workers), with an ugly fallback for IE9.

padolsey commented 11 years ago

I like the idea of only using promises if the native API is available (having to include a promise lib as a dependency was why I avoided it thus far). +1 from me, I'll take a look.

evanworley commented 11 years ago

Yeah I hear you, and I appreciate that there are no external dependencies. It might be tricky to support folks that don't want promises, and people who want a jquery deferred.

darsain commented 11 years ago

@evanworley jQuery.Deferred is not a valid Promises spec implementation :) Or at least it wasn't last time I checked (few months ago).

evanworley commented 11 years ago

I'm fine with detecting and using native/polyfilled promises, I can work with that.

Raynos commented 11 years ago

If you support promises please support both promises & callbacks.

padolsey commented 11 years ago

Ok, here's what I am proposing to solve the whole callbacks/promises issue:

There will be three ways to "fulfil" (successfully return) something from within the worker.

Direct

var o = operative({
  foo: function() {
    return 123;
  }
});
o.foo(function(err, result) {
  result; // => 123
});

Async Basic

var o = operative({
  foo: function() {
    var finish = this.async():
    // do async stuff ...
    finish(456);
  }
});
o.foo(function(err, result) {
  result; // => 456
});

Async Promise

var o = operative({
  foo: function() {
    var resolver = this.deferred():
    // do async stuff ...
    resolver.fulfil(456);
  }
});
o.foo().then(function(result) {
  result; // => 456
});

There will be two different ways to reject from within a worker:

Via Exception

var o = operative({
  foo: function() {
    // ...
    throw new Error('something');
  }
});
o.foo(function(err, result) {
  err; // => 'something'
});

Via reject() (promise)

var o = operative({
  foo: function() {
    var resolver = this.deferred();
    // ...
    resolver.reject('something');
  }
});
o.foo().then(function() {
  // fulfil callback (not called in this case)
}, function(err) {
  err; // => 'something'
});

We will detect if you use async() or deferred() from within an operative method and ensure that we don't expect a direct return in that case.

Assuming this can all be implemented (I'm already in the process of doing this in a branch) is everyone happy with this API?

I guess this'll have to be a 1.0.0 release because technically these are backwards-incompatible changes (specifically the cb(err, res) (#5) change).

padolsey commented 11 years ago

After posting that I realised that we'll probably want event a direct return to fulfil a promise too? If so, that's gonna be tricky to reconcile with the existing regular-callback style. To be honest I prefer the explicit this.deferred() style because it means we can be sure that we're NOT expecting a direct-return and that we're NOT gonna have to worry about calling any callbacks. Let me know your thoughts...

evanworley commented 11 years ago

@padolsey The pull request looks good. Regarding your concern above for the direct return, I'd advocate for the same behavior whether or not any asynchronous code is executing inside the web worker. This way if the operative code changes in the future, say a synchronous block becomes asynchronous, not much will need to change.

padolsey commented 11 years ago

Merged