Skip to content

Conversation

TomSmith27
Copy link
Contributor

This change will allow a client to listen to a single event callback, instead of having to pass in functions for all options on the Hub.

Also updated the README to reflect this,

And also added the default as Null instead of undefined for typescript as i think null is a better indication than undefined, and aligns with C# more

README.md Outdated
}

unregisterCallbacks(implementation: IChatHubCallbacks) {
this.connection.off('Welcome', implementation.welcome);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As i mentioned in other PRs this will not work because "this" in the welcome call will not be "implementation" (I think)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok good point i have reverted my changes for the register and unregister callback methods

Copy link
Owner

@RicoSuter RicoSuter May 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to internally store the registered lambdas because the off-methods will "dynamically" create new lambda which do not have the same identity (ie not ===) as the registered one and thus they do not unregister anything - or am I wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the best approach might be to write some unit tests around this topic so we can make sure everything works as intended.

@TomSmith27
Copy link
Contributor Author

Do you think we should merge the changes for the single callbacks in and raise an issue to address the register unregister for the the ICallback implementation.

I know the single onCallbacks work because i am using them in my project

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants