-
-
Notifications
You must be signed in to change notification settings - Fork 220
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
feat: move arrpc server into a worker thread #1053
base: main
Are you sure you want to change the base?
Conversation
8feecf6
to
daec102
Compare
invite events are simply opening https://discord.gg/D9uwnFnqmd in your browser of choice and it sending that to your open client. The callback is so the website can display its completed or error thing Same case as #1016 |
Oh, I thought it was game invites, lol. Thanks for the info. Let me see what's up. It should be fixable, I think |
daec102
to
03f97d4
Compare
Off by one error, works on my side now |
Instead of writing an insane review with github web editor heres a patch. It's functionally the same but doesn't have the hardcoded types (?) or add a new dep |
a4c806a
to
bec9764
Compare
Thanks. I removed the new dependency in my latest revision. Not sure why we wouldn't want the types? |
bec9764
to
47e064f
Compare
47e064f
to
f46f316
Compare
You're over complicating it by quite a bit, sending the data over postMessage is fine, you don't need to handle the port stuff yourself & worker.on error exists so you dont need a try catch. Also please stop forcing pushing it makes it insane to follow changes 😭 |
Agree to disagree, I suppose. It's much more readable to me with the MessageChannel, and adds like... 1 line of code, which is why I didn't use You also use a map instead of an array for storing the invites, which is fine too, but I don’t really see why you’d need an id more unique than a monotonically increasing array index for that. Maybe it’s more extendable.
Sure. Habit from other projects I contribute to where this is the expected workflow. I just use the "see changes since last revision" button. |
Solution for #1009, which I can reliably reproduce without this change.
I have another version of this which instead works with this arrpc PR OpenAsar/arrpc#133, but it seems like Vesktop might be the appropriate place to put this complexity.
I have tested that game status RPC works as expected but I haven't found a game with which to test invites.