FromEventPattern does not conform to v4

[mattpodwysocki]

The current implementation of fromEventPattern deviates from the current v4 implementation in several ways:

• removeHandler is no longer optional
• removeHandler does not get return value from addHandler

Is the intent here to follow this behavior, or deviate? If we are to deviate, then we must ask why the addHandler is addHandler: (handler: Function) => any instead of addHandler: (handler: Function) => void if we aren't returning anything, nor are we using the value produced from it.

RxJS version: v5 - current

Code to reproduce:

This is an example of using RxJS v4 with the addHandler returning a value, which would then be forwarded onto the remove handler.

Observable.fromEventPattern(
  (h) => { return elem.connect('data', h); },
  (_, r) => { r.disconnect(); }
);

Expected behavior:

The expected behavior would forward the return value from elem.connect('data', h) so that you could remove the handler from the value by calling disconnect.

Actual behavior:

Currently, RxJS v5 does not support doing anything with the addHandler despite its signature of allowing anything to be returned, hence why its signature of addHandler: (handler: Function) => any is confusing if nothing is ever done with the return value.

Additional information:

If this is indeed breaking change behavior, we should put that in the documentation and the reasons for this. The reason it was added originally was that some libraries instead pass a subscription like object to the user which you can later disconnect instead of the standard Subject/Observer pattern with add/remove handlers.

[benlesh]

cc/ @trxcllnt who I believe is the original implementer. (this was added when RxJS was v 2 or 3 I think)

[trxcllnt]

@Blesh I don't know how I got associated with implementing this, I've literally never touched this file. I'm halfway convinced you're messing with me :P

@mattpodwysocki I agree 100%, this should exactly match v4 behavior, something I would've done if I was the author of this version cough@Bleshcough

[jayphelps]

I think @trxcllnt has been playing with git-blame-someone-else

[trxcllnt]

@jayphelps oh man good show. @Blesh #95

[paulpdaniels]

I just ran into this while writing a wrapper for a Redux store. Fortunately in that case there was a simple work around of Observable.from(store).map(() => store.getState()), but still 💯

[benlesh]

Oops, sorry @trxcllnt... for some reason I thought you had done the original work on this way back when we merged what you had done with what I had done when this was called "RxNext" or whatever. haha.

[kwonoj]

Closing via #2224

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.