-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
L2l #1
base: master
Are you sure you want to change the base?
L2l #1
Conversation
… clients are added correctly
…ee how many channels it is still in
…r status before killing it
…l is brought online
…master with unit tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Matt, thanks for the PR! Can you please check my comments of your code? Thanks!
if (typeof senderRecvrA[onReceivedMethodA] !== "function") throw new Error(`sender a has no receive method ${onReceivedMethodA}!`); | ||
if (typeof senderRecvrB[onReceivedMethodB] !== "function") throw new Error(`sender b has no receive method ${onReceivedMethodB}!`); | ||
this.senderRecvrA = senderRecvrA; | ||
this.senderRecvrA.l2lclient ? {} : this.senderRecvrA.l2lclient = Channel.makeL2LClient() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this structure? Why not simply an if instead of a tertiary with an emtpy object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected with ifs
import L2LClient from "lively.2lively/client.js"; | ||
import { string, num, promise, fun } from "lively.lang"; | ||
|
||
export class Channel { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to copy the channel implementation from channel.js? Does subclassing work? Or just configuring the original channel with a variable backend? (l2l vs local)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved with inheritance
async getSessions(senderA,senderB,ackFn){ | ||
var l2lA = senderA.l2lclient, | ||
l2lB = senderB.l2lclient | ||
var returnVal = 'incomplete'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is returnVal
? It is never used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
} | ||
|
||
async getSessions(senderA,senderB,ackFn){ | ||
var l2lA = senderA.l2lclient, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you define l2lA
but it is never used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
console.log('senderRecvrB not disconnected'); | ||
} | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the procedure for cleaning up l2lA and l2lB be the same? it seems strange that you have to query for getSessions to disconnect b?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's because I need to make sure it's the last thing in the room before it disconnects, since it's the master.
} | ||
|
||
if (this.senderRecvrB && this.senderRecvrB.l2lclient) { | ||
this.getSessions(this.senderRecvrA,this.senderRecvrB,(a)=> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use async/awai as much as possible. It makes code much more readable than callback style
|
||
l2lA.sendTo(l2lA.trackerId,'joinRoom',{roomName: l2lB.socketId.split('#')[1]}) | ||
l2lB.sendTo(l2lB.trackerId,'joinRoom',{roomName: l2lB.socketId.split('#')[1]}) | ||
this.online = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to wait for acknowledgment for joining the rooms to be sure we are really only? In other words, what happens when joinRoom fails? I cannot find error handling somewhere
send(content, sender) { | ||
|
||
|
||
return this.deliver(sender); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"deliver" was used in the local channel to implement the details of send, here it is empty so a send actually does nothing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had just emptied it with the intention of filling it with code particular to the l2l version, so this should be resolved with inheritance now
await testChannel.senderRecvrA.l2lclient.whenRegistered(300) | ||
await testChannel.senderRecvrB.l2lclient.whenRegistered(300) | ||
|
||
window.testChannel = testChannel; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clean up debug code? Leaving globals and such around might lead to strange effects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
expect(masterBuffer[0].payload).equals(ack,'payload not correct') | ||
}) | ||
|
||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of those tests actually runs lively.sync. How would the tests ""messaging between master and client - single op" and "syncing master with two clients - simple prop" look like when using l2l?
…e goOnline method
…t behavior with l2lchannel
…or synchronous checking
L2L Channel Framework in place. Send/Receive now register and need to be tied to the channel send/receive methods.