Bugfix/unregister callbacks #29
Conversation
* unregisterCallbacks method added to template * Replacing tabs with spaces * README updated to reflect changes in generated code
| @@ -1,4 +1,8 @@ | |||
| export class {{ Name }}Hub { | |||
| {% for operation in Callbacks -%} | |||
| callbackFor{{ operation.Name }}: ({% for parameter in operation.Parameters %}{{ parameter.Name }}: {{ parameter.Type }}{% if forloop.last == false %}, {% endif %}{% endfor %}) => void; | |||
There was a problem hiding this comment.
To be safe this should be an array of registrations so if multiple impls are registered all callbacks are unregistered, not only the last one. But we can also leave it as is as this is an edge case.
There was a problem hiding this comment.
Modifications were made following your comments. Please check.
|
@TomSmith27 is this still relevant? Can you have a look? |
|
@RicoSuter, @TomSmith27 this is still relevant. Let me resolve the conflict and update the PR. |
|
@beorosz I think the problem with this implementation is it only allows a single callback to be registered for each event. If two places in the app set callbackForWelcome wont one of them be overidden? I have also created a pull request that addresses a similar issue Could you take a look at that and let me know what you think? |
|
@TomSmith27 yes, your solutions looks more flexible. @RicoSuter what do you think? |
|
@TomSmith27 I think if you only register the function and do not create a new lambda, then "this" is not correctly scoped within the call... |
|
I am currently testing this is in a project and the below examples seem to be working, i am getting the correct context for Is there another situation i can test to make sure it works in all situations const receiveMessage = (name: string, age: number) => {
this.person.name = name;
this.person.age = age;
};
chatHub.onReceiveMessage(receiveMessage);
chatHub.onReceiveMessage((name: string, age: number) => {
this.person.name = name;
this.person.age = age;
}); |
|
@RicoSuter @TomSmith27 should I close this PR then? |
Please review the bugfix PR. I've created a dummy Angular app for testing the register/unregister functions, I can push it into this fix if you think it could be useful.