search

tj / axon

message-oriented socket library for node.js heavily inspired by zeromq
MIT License
1.49k stars 155 forks source link

Simple unsubscribe from event implementation. #101

Closed porkchop closed 11 years ago

porkchop commented 11 years ago

Hi everybody. I found this feature necessary for myself and I noticed some +1erry for it in the issues, so here it is. :) I'm happy to make adjustments if this isn't satisfactory. Further, a race condition between publisher and subscriber readiness prevented some of the tests from completing (for me), so interlocked the publisher emit sequences with the publisher socket on connect event for these tests.

gjohnson commented 11 years ago

Nice, @visionmedia will probably know, but any reason why the regex equality check couldn't just be a simple string comparison (r1.toString() == r2.toString()) ?

tarqd commented 11 years ago

@gjohnson I believe it's because some on engines toString() doesn't normalize the regex modifiers order so this would return false (/foo/gi).toString() == (/foo/ig).toString() even though they are the for all intents and purposes equal

porkchop commented 11 years ago

@gjohnson, that's what I understand as well. Actually I wasn't sure if that would approach would always work, so I consulted popular opinion. See http://stackoverflow.com/questions/10776600/testing-for-equality-of-regular-expressions where I hijacked that regexSame function. It has two +1's which basically means it's gotta be right :P Now, looking at the version just below mine actually looks a bit more concise to me, hmm...

porkchop commented 11 years ago

It just occurred to me that toString comparison might be more appropriate regardless. Trying to determine if two regexes are entirely equal is pretty much a hopeless task anyways. Like /[bar]/g and /[rab]/g are effectively the same too. I suggest that this should be up to the user to track when registering for and deregistering from events. Which seems to say to me that /foo/gi should not match up with /foo/ig. Hence toString equality would in fact be more correct for the use case??

tarqd commented 11 years ago

I agree with just using toString equality in this case (and in v8 /foo/gi and /foo/ig both end up as "/foo/gi" anyway)

porkchop commented 11 years ago

Agreed guys. Here's the updated version

gjohnson commented 11 years ago

Looks good to me.

tj commented 11 years ago

LGTM as well!

tj commented 11 years ago

thanks @porkchop

porkchop commented 11 years ago

My pleasure guys! So glad to help out :)