RicoSuter / SigSpec

Specification and code generator for SignalR Core.
MIT License
159 stars 37 forks source link

Bugfix/unregister callbacks #29

Open beorosz opened 4 years ago

beorosz commented 4 years ago

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.

RicoSuter commented 4 years ago

@TomSmith27 is this still relevant? Can you have a look?

beorosz commented 4 years ago

@RicoSuter, @TomSmith27 this is still relevant. Let me resolve the conflict and update the PR.

TomSmith27 commented 4 years ago

@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?

32

beorosz commented 4 years ago

@TomSmith27 yes, your solutions looks more flexible. @RicoSuter what do you think?

RicoSuter commented 4 years ago

@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 think there is a reason I didnt register the function directly...

TomSmith27 commented 4 years ago

@RicoSuter

I am currently testing this is in a project and the below examples seem to be working, i am getting the correct context for this.

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;
});
beorosz commented 4 years ago

@RicoSuter @TomSmith27 should I close this PR then?