search

BlueBubblesApp / bluebubbles-server

Server for forwarding iMessages to clients within the BlueBubbles App ecosystem
https://bluebubbles.app
Apache License 2.0
540 stars 47 forks source link

adding back explicit bind #631

Closed dltacube closed 6 months ago

dltacube commented 6 months ago

don't tell me you already fixed this on your machine :D

dltacube commented 6 months ago

Tested locally and it works.

zlshames commented 6 months ago

Yep, on my development branch, sorry... i pushed it last night

dltacube commented 6 months ago

Oh ok. Why didn't you use my original PR then?

zlshames commented 6 months ago

Oh ok. Why didn't you use my original PR then?

Cuz I had already fixed it before you had submitted the PR

dltacube commented 6 months ago

Are you talking about this commit? https://github.com/BlueBubblesApp/bluebubbles-server/commit/2e72faa0ded787f365e5d122bc2f2206f352ec5f

A few of us got around to testing that change and it wasn't emitting websocket messages.

zlshames commented 6 months ago

Are you talking about this commit? 2e72faa

A few of us got around to testing that change and it wasn't emitting websocket messages.

But the .bind code was? cuz they should be essentially doing the same things

dltacube commented 6 months ago

Yes, the .bind code is emitting messages. I'm happy to walk you through how I'm testing this but in brief, I'm looking at the raw network traffic in a terminal. I also assumed your change did the same as the bind method which is why I didn't jump to test it right away.

zlshames commented 6 months ago

Yes, the .bind code is emitting messages. I'm happy to walk you through how I'm testing this but in brief, I'm looking at the raw network traffic in a terminal. I also assumed your change did the same as the bind method which is why I didn't jump to test it right away.

i dont think i need a walkthrough, im just confused why .bind would work, but the () => ... wouldn't. Are you 100% positive it's not working, and 100% positive it was working with bind? the handleUpdatedMessage function was being called for me when I was using the arrow func

dltacube commented 6 months ago

Yea 100% sure. Donovon tested it as well independently which is what prompted me to compile that version.

I can't say for sure why one is working and the other isn't but I'm looking into it. I asked you about closures in discord last night :D

zlshames commented 6 months ago

Yea 100% sure. Donovon tested it as well independently which is what prompted me to compile that version.

I can't say for sure why one is working and the other isn't but I'm looking into it. I asked you about closures in discord last night :D

So was the handleUpdatedMessage not even called with the closure?

dltacube commented 6 months ago

Yea 100% sure. Donovon tested it as well independently which is what prompted me to compile that version. I can't say for sure why one is working and the other isn't but I'm looking into it. I asked you about closures in discord last night :D

So was the handleUpdatedMessage not even called with the closure?

I'd have to look again to be 100% sure since I resorted to running a websocket client and looking at that output. Although I believe I tried arrow functions myself and at the time didn't see the handleUpdateMessage function run at all.

dltacube commented 6 months ago

I'm gonna butcher the explanation I'm seeing but arrow functions inherit their this context while bind creates their own, presumably by taking a snapshot of the class/instance context. This is why I asked about closures coming into play. Maybe then there is a better fix if we assume that the context shouldn't be changing to the point where arrow functions stop working?

zlshames commented 6 months ago

oh i see why... my fault. pushed another update... 🤦

dltacube commented 6 months ago

https://github.com/BlueBubblesApp/bluebubbles-server/commit/703707aac7f192ea7e18cd9e0ac9a9132894ccd9 for reference.