kefirjs / kefir

A Reactive Programming library for JavaScript
https://kefirjs.github.io/kefir/
MIT License
1.87k stars 97 forks source link

Don't release Zalgo! #22

Closed tehsenaus closed 9 years ago

tehsenaus commented 9 years ago

Kefir releases Zalgo. See here for why that is bad: https://github.com/oren/oren.github.io/blob/master/posts/zalgo.md

This test case demonstrates the problem:

var s = kefir.fromBinder(function (emitter) {
    emitter.emit('test');
});

var value;

// Could be async, therefore MUST be async
s.onValue(function (v) {
    value = v;
});

value = 'initial';

setTimeout(function () {
    value.should.equal('test');
}, 10);

I just spent the best part of 2 hours tracking down a bug caused by this.

rpominov commented 9 years ago

Not sure about this. I think users should have that control and do things sync when they need to.

I'd recommend to force async in the event source end, i.e. do

kefir.fromBinder(function (emitter) {
    setTimeout(function() {emitter.emit('test')}, 0);
});

... if you need to emit values right after subscription. But the fact itself that you need this might be a sign of bad design.

Also it not very easy to change that behaviour in the library. Such change could have a lot of unexpected consequences in very unexpected places.

Btw, Bacon and RxJS works exactly same way.

tehsenaus commented 9 years ago

I disagree (and so would issacs - read his article). This behaviour is a huge source of bugs. Right now, if I have code like this:

stream.onValue(function (v) {
    value = v;
});

value = 'initial';

... there is no way to know which statement will be executed first, without knowing what 'stream' is. On a large multi-developer project, a developer cannot be expected to know every event source. This is the kind of thing with which I expect an interface to be consistent.

rpominov commented 9 years ago

There are 3 solutions for this, that comes to my mind:

1) Always assume that callback might be executed synchronously. In this case you should write that snippet like this:

value = 'initial';

stream.onValue(function (v) {
    value = v;
});

2) Protect yourself with .delay(0) if you need to rely on asynchrony, e.g.

stream.delay(0).onValue(function (v) {
    value = v;
});

value = 'initial';

3) Make a convention in your team, that all streams should be asynchronous, e.g. sync emitter.emit() in .fromBinder() callback is not allowed.

I'd go for 1, and 3 is a good practise also. Actually I do 1 when using any library and have no problems with that.

tehsenaus commented 9 years ago

I came up with a better workaround, by wrapping onValue like below:

function onValue(stream, callback) {
    process.nextTick(function () { stream.onValue(callback); });
}

Similarly for onEnd, etc. Now the library is Zalgo-safe.

I still think this should be made consistent in the library though. Performance isn't an issue - take a look at Bluebird promises as an example of an efficient Zalgo-safe library.

rpominov commented 9 years ago

Still don't think we should force async everywhere. I like your solution though, use it if you need to :)