Skip to content
This repository has been archived by the owner on Feb 11, 2020. It is now read-only.

using newPacket instead of packet for publish #403

Merged
merged 1 commit into from
Feb 22, 2016

Conversation

paularmand
Copy link

@mcollina as requested a PR for the proposed change.

@mcollina
Copy link
Collaborator

What are you trying to achieve with this? I remember there was a reason why I added that newPacket, but tests are passing anyway.

@paularmand
Copy link
Author

Exactly that, newPacket isn't used anymore in the original code, only packet is used, making the creation of the newPacket ineffective. As I understand javascript scoping, packet is within scope of the publish function, so that's why it's still working, but any change to the newPacket variable will not be propagated. The reason I'm stating this is that I was trying to help out in #368 , and was suggesting to change the payload in the newPacket variable to the OP.

@mcollina
Copy link
Collaborator

@paularmand
Copy link
Author

True, newPacket is hence used for the persistence layer, but the original packet is still published via ascoltatori.

@mcollina
Copy link
Collaborator

But that won't help overriding the payload. These needs to be changed as well: https://github.com/mcollina/mosca/blob/master/lib/client.js#L106-L107.

@paularmand
Copy link
Author

Yes, that's quite possible. I 've had to change the forward function too in client.js for my use case. Something that I already pointed out to the OP in #368. I was referring there to the fact that the client.js file has it's own packet structure, and uses that.

mcollina added a commit that referenced this pull request Feb 22, 2016
using newPacket instead of packet for publish
@mcollina mcollina merged commit 6d5fc48 into moscajs:master Feb 22, 2016
@mcollina
Copy link
Collaborator

Merged and released :)

@paularmand paularmand deleted the fix/packet_newpacket branch April 12, 2016 15:42
@paularmand
Copy link
Author

Thanks !

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.

2 participants