openbazaar-go
openbazaar-go copied to clipboard
[WIP] (#1851) Send offline messages through web relay if configured
This does not address the config file migration so for now to test this you need to add this to the bottom of your config file:
"WebRelays": [
"wss://webchat.ob1.io:8080"
]
Offline messages will now also be sent through the configured web relay using websockets with this PR.
Solves: #1851
Testing:
- Spin up two new nodes with fresh data directories on two different machines
- If possible block incoming connections to those nodes to simulate mobile (or just use mobile)
- Start both up
- Send chat messages or order messages between the nodes and check the logs for OFFLINE_RELAY messages
Parent issue: https://github.com/OpenBazaar/openbazaar-go/issues/1851
Excellent! @hoffmabc will you be making addition commits for this PR to add the offline message listener via the relay?
@hoffmabc bump
@hoffmabc if we're pursuing this, I'd like to see the following improvements after doing a quick scan through:
- Update the PR description indicating what impact these changes will have. Maybe a use case illustrating their need. And perhaps instructions for setting this up for independent testing/verification. (Something that can later be used for updating our official docs.)
- Error handling inside the relay manager.
- Unit tests around the relay manager and the new config getter.
- Travis passing.
Thinking about this further, I like that this was produced as a separate manager from a pattern perspective. However, I think that our message sending is getting spread across a few different parts of our app and this implementation might be improved if we take a second to refactor our message sending behavior to support sending a message via multiple transports (a message mux of sorts).
I don't think we're in a hurry to deliver this, perhaps we can reconsider our approach before digging into unit tests and error handling as mentioned before? /cc @drwasho @hoffmabc
I'm fine with whatever approach you guys want to take. I'll defer to @drwasho on this one.
Coverage decreased (-0.3%) to 38.148% when pulling 8b3d2b7ab0edc6e710c8162495fe5cd4d82cb8ab on brian.use-relay into b80611d177f7898d3f5fef9c1c30f62415421674 on master.
Thinking about this further, I like that this was produced as a separate manager from a pattern perspective. However, I think that our message sending is getting spread across a few different parts of our app and this implementation might be improved if we take a second to refactor our message sending behavior to support sending a message via multiple transports (a message mux of sorts).
I don't think we're in a hurry to deliver this, perhaps we can reconsider our approach before digging into unit tests and error handling as mentioned before? /cc @drwasho @hoffmabc
Perhaps this refactor to mux model could be a larger, broader refactor later on? I think that pursuing that in this PR would probably expand the scope and prevent rollout for quite a while.