search

ngrx / store-devtools

Developer Tools for @ngrx/store
MIT License
326 stars 38 forks source link

Fix unsubscribing from the extension #37

Closed zalmoxisus closed 7 years ago

zalmoxisus commented 7 years ago

Fix #33.

MikeRyanDev commented 7 years ago

Thanks!

rob3c commented 7 years ago

Hi guys, thanks for addressing this! I have one small comment you may want to consider:

Just returning connection.unsubscribe directly doesn't bind this when it's later invoked by the subscriber, which leaves the code open to a possible regression later if connection's unsubscribe implementation changes (e.g. it referencing this.subscribers or something). That's why I returned a lambda instead in my suggested fix in https://github.com/ngrx/store-devtools/issues/33#issuecomment-257005059

return () => connection.unsubscribe();

Of course, an alternative is return connection.unsubscribe.bind(connection).

zalmoxisus commented 7 years ago

@rob3c, sorry, I didn't notice that you already suggested the solution (it would save me a lot of time investigating).

A better solution would be just to return the result of subscribe as it already gives the unsubscribe function:

return connection.subscribe(change => subscriber.next(change));

It's also in the spirit of redux. The second unsubscribe was meant to remove all the instances from the current connection (that's why it had an argument), but it's not used anymore and has the same result as one above.

If you have any suggestions about the API, let me know.

rob3c commented 7 years ago

@zalmoxisus Ah, good point! That's even simpler while still avoiding the binding issues. (It looked a little weird that unsubscribe needed to be referenced separately on connection, but I figured that was just due to my Rx-style Observable contract bias.)

Oh, and thanks again for your awesome tools! I don't use Redux directly right now, but it's nice that they work so well with ngrx, too :-)