-
Notifications
You must be signed in to change notification settings - Fork 131
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
Update for OBS Websocket 5.0 #335
Conversation
201f330
to
3a75a9c
Compare
specifically call out needing version 5+
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.
Looks fine for the most part but have a few nits.
@@ -1,62 +1,77 @@ | |||
import { EventEmitter } from "events"; | |||
import OBSWebSocket from "obs-websocket-js"; | |||
import { isIPv6 } from "net"; |
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.
If this function exists (as well as isIPv4
) in the built in net
package, how come we need to install the is-ip
package?
if (chunks.length !== 4) { | ||
return false; | ||
} | ||
import { isIPv4, isIPv6 } from "is-ip"; |
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.
import { isIPv4, isIPv6 } from "is-ip"; | |
import { isIPv4, isIPv6 } from "net"; |
What's the difference from just importing these from the net
package?
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.
we can't use net.isIP
from renderer. I could add IPC but that seems overkill for this.
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.
Oh right. We could consider exposing it over the context bridge, similar to how to expose the the electron-log functions. E.g.
// inside of preload.ts
log: log.functions,
net: { isIPv4, isIPv6 },
and then in the actual usage of it:
const { isIPv4, isIPv6 } = window.electron.net;
Although it does reduce a dependency, it also makes it harder to test logic and components since it now requires them to be injected over context bridge.
I'll defer it to you, but I'm happy to approve this change once the tests/build is passing.
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.
LGTM once the build/tests are passing.
So the autoswitcher works with OBS 28+.
Still need to test with 2 PCs.