-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make subscribe with context less error prone #12
Comments
Yeah, I look at it now and I agree that pattern of using an array was array odd and I would be open to changing it. Personally, I don't really use context and just simply use a bind method. IE: radio(CHANNEL).subscribe(angular.bind(this, this.mymethod); |
Ah, I see. I don't use angular, so don't have the bind method available. I Regards, Mark On Fri, May 22, 2015 at 6:20 PM, Scott (Tomoharu) Murphy <
|
Angular was just an example but Bind is available in ES5 so if you just need to support IE9 and up, it is natively available already. http://itsnull.com/articles/start-using-es5/ (check Function.prototype.bind). If you need to support older browsers, there are plenty of bind solutions like lowdash/underscore e.t.c. For radio and making it better, I think we can just keep it simple and an provide an alternative to that .subscribe([method, context]) and make the following available: radio(constant.SHOW_CLIENT_APPLICATION).subscribe(this.xyz, this); which is much more natural. |
Yes, sounds good! Mark On Sat, May 23, 2015 at 5:34 PM, Scott (Tomoharu) Murphy <
|
Okay, will try to find time for this sometime. But if you have time, feel free to tackle it as well. |
Hi,
A number of times now I've done stuff like this:
radio(constant.SHOW_CLIENT_APPLICATION).subscribe(this.xyz, this);
instead of this:
radio(constant.SHOW_CLIENT_APPLICATION).subscribe([this.xyz, this]);
Would you please create a new method/alias to prevent this kind of thing? Maybe something like:
radio(constant.SHOW_CLIENT_APPLICATION).subscribe(this.xyz).context(this);
I know its minor, but its a big problem when you first run into this as there are no error/indications to help out.
Thank you,
Mark
The text was updated successfully, but these errors were encountered: