Skip to content
This repository has been archived by the owner on Jul 7, 2022. It is now read-only.

Update websockets.ios.js #54

Closed
wants to merge 2 commits into from
Closed

Update websockets.ios.js #54

wants to merge 2 commits into from

Conversation

ineiti
Copy link
Contributor

@ineiti ineiti commented Sep 12, 2018

This patch allows to correcly run nativescript-websockets on iOS. Without this patch,
the same objects are referenced when passed to this._socket.send, thus the websocket resend
previous messages.

This patch allows to correcly run nativescript-websockets on iOS. Without this patch,
the same objects are referenced when passed to `this._socket.send`, thus the websocket resend
previous messages.
@ineiti ineiti mentioned this pull request Sep 12, 2018
@NathanaelA
Copy link
Owner

Thank you for your PR, I really do appreciate PR's. However, this patch will actually cause it to crash for several reasons. (i.e. passing in null, undefined) You would need to move this below the null check, for it to work safely and not break other peoples apps.

However, can you explain exactly what you are trying to do, I don't see the point in making a clone of the value unless you are guilty of reusing the message variable yourself. All it does is add memory wasting performance hit. If you are reusing the variable; then you won't get old messages sent -- you will get new messages sent twice as you would replace the value inside the queue. So for the queue I can see some justification to clone the value before pushing it into the array, so I can see this being added to around line 333, before the message is pushed onto the queue.

Put a copy of the message in the queue, after all the tests.
@ineiti
Copy link
Contributor Author

ineiti commented Sep 12, 2018

What you say makes sense. I got to admit I copy/pasted a fix we have in our code that came from a student. And we do some protobuf magic before sending it out on the websocket, so it might be that the message gets reused. Even though I cannot recall if the message got queued or not...

Anyway - I added the .slice to the two places where the message gets put in the queue or sent, to make sure you can send the same modified buffer twice. Does that look better?

@NathanaelA
Copy link
Owner

Thank you I have added these patches into both ios and android sides. So it is in version 1.5.3

@NathanaelA NathanaelA closed this Oct 10, 2018
@jeffallen
Copy link

Just to help the next guy who is trying to understand what happened to these changes, they are in f519c19.

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

Successfully merging this pull request may close these issues.

None yet

3 participants