padolsey / operative

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

Events #15

Closed kobezzza closed 9 years ago

kobezzza commented 11 years ago

Really need something like:

var worker = operative(function (callback) {
    for (var i = 0; i < 1e9; i++) {
        if (i == 1e9 / 2)  {
             trigger('progress', '50%');
        }
    }

    callback('im done!');
});

worker.on('progress', function (val) { ... });
worker(function (val) { ... });
padolsey commented 11 years ago

I agree that there is a need for Operative to support the idea of progress. I have been playing with a few things, and tbh, I'm not convinced that adding an EventEmitter and a progress event is the best way to go. One simple alternative would be to allow the provided callback to be called multiple times. So you could potentially do:

var worker = operative(function(callback) {
    for (var i = 0; i < 1e9; i++) {
        if (i == 1e9 / 2)  {
             callback('progress', '50%');
        }
    }

    callback('complete');
});

worker(function(state, v) {
    if (state === 'progress') { ... }
    else if (state === 'complete') { ... }
});

The benefit with this is that:

The only potential implementation problem with this is that, for the operative instance, it becomes unclear when it can delete the callback (freeing up memory) -- usually it'll delete it when the callback is first-called, but if the callback supports multiple calls it will just have to stick around on the operative instance until the operative itself is destroyed via operativeInstance.terminate().

Welcoming suggestions/thoughts...

kobezzza commented 11 years ago

Maybe create two different constructor?

For single usage

operative

For multiply usage (with "terminate" for destroy)

operative.connect
chrisritter commented 10 years ago

Any hope for an events api?

evanworley commented 10 years ago

@padolsey - I like the simplicity of the callback approach, and using a simple JSON object the developer can communicate data as structured as they like. I think it's totally acceptable to keep the callback around until terminate is called.

chrisritter commented 10 years ago

I like that approach

chrisritter commented 10 years ago

@padolsey -- any interest on changing the callback to continue until termination?

padolsey commented 10 years ago

Yes, that sounds reasonable. It may end up being an opt-in thing though. I'll play with it a bit and shall try to work something into the next release (which will hopefully include some more robust testing via BrowserStack).

samzhao commented 10 years ago

Is it possible to just bring the postMessage method into Operative?

There's also the Promise deferred.notify() method if we decide to use Promise.

padolsey commented 10 years ago

@samzhao AFAIK notify is not supported in ES6 Promises (nor its shims), so I don't think it'd be wise to support that (it'd mean operative would have a require a specific Promise implementation). Please correct me if I'm wrong here though.

Re: postMessage: You can still utilize postMessage on the underlying worker via operativeInstance.worker.postMessage(...). For operative's actual API, I think I prefer the callback approach for progress-events -- but I'm curious if there are merits to a generic per-operative postMessage as opposed to per-operative-method calls/callbacks?

samzhao commented 10 years ago

@padolsey, ah sorry I didn't know notify won't be supported in ES6. I'll try the postMessage.

And I personally think the generic operative postMessage is good enough. To distinguish between instances, we can just pass in a different message each time like so:

var a = Operative({
    var data = "foo";
    postMessage("progress-a", data);
});

var b = Operative({
    var data = "bar";
    postMessage("progress-b", data);
});

Operative.on("progress-a", callback);
Operative.on("progress-b", callback);
padolsey commented 10 years ago

As of 0.4.0-rc1 (currently available via npm) -- callbacks are no longer destroyed until the operative itself is terminated, so stuff like this will work fine:

var op = operative(function(callback) {
    for (var i = 0; i < 1e9; i++) {
        if (i == 1e9 / 2)  {
             callback('progress', '50%');
        }
    }

    callback('complete');
});

op(function(state, v) {
    if (state === 'progress') { ... }
    else if (state === 'complete') { ... }
});

@samzhao Re: postMessage -- I think that might add to the complexity of messaging using operative (especially for multi-method operatives). As it is, there is already callbacks and promises -- and operative takes care of passing things to the correct method within the worker context. A generic postMessage seems to produce little gain over changing the mechanics of callbacks (which is what I ended up doing). Happy to discuss further though -- as 0.4 is only in "release-candidate" at the moment.

samzhao commented 10 years ago

Thanks James! I think this is good :) And I agree that postMessage might not be much beneficial.

On Sat, Oct 18, 2014 at 1:03 PM, James Padolsey notifications@github.com wrote:

As of 0.4.0-rc1 (currently available via npm) -- callbacks are no longer destroyed until the operative itself is terminated, so stuff like this will work fine:

var op = operative(function(callback) { for (var i = 0; i < 1e9; i++) { if (i == 1e9 / 2) { callback('progress', '50%'); } }

callback('complete');

});

op(function(state, v) { if (state === 'progress') { ... } else if (state === 'complete') { ... } });

@samzhao https://github.com/samzhao Re: postMessage -- I think that might add to the complexity of messaging using operative (especially for multi-method operatives). As it is, there is already callbacks and promises -- and operative takes care of passing things to the correct method within the worker context. A generic postMessage seems to produce little gain over changing the mechanics of callbacks (which is what I ended up doing). Happy to discuss further though -- as 0.4 is only in "release-candidate" at the moment.

— Reply to this email directly or view it on GitHub https://github.com/padolsey/operative/issues/15#issuecomment-59627711.