Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pendo324
Copy link

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.

@Covkie
Copy link
Collaborator

Covkie commented Jan 16, 2025

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

@Covkie
Copy link
Collaborator

Covkie commented Jan 16, 2025

Screenshot_20250115_193917
anyway yeah doesn't work for invites

@pendo324
Copy link
Author

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

@pendo324
Copy link
Author

Off by one error, works on my side now

@Covkie
Copy link
Collaborator

Covkie commented Jan 16, 2025

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

buh.patch

@pendo324 pendo324 force-pushed the threaded-arrpc branch 2 times, most recently from a4c806a to bec9764 Compare January 16, 2025 16:36
@pendo324
Copy link
Author

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

buh.patch

Thanks. I removed the new dependency in my latest revision. Not sure why we wouldn't want the types?

@Covkie
Copy link
Collaborator

Covkie commented Jan 17, 2025

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.
patch.txt

Also please stop forcing pushing it makes it insane to follow changes 😭

@pendo324
Copy link
Author

You're over complicating it by quite a bit

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 postMessage. I’m surprised your code works using a .ts file for the worker, even when it isn’t explicitly loaded using an import/require (you reference the js output, which I’m surprised is included by ESBuild in the bundle when it’s not ever imported). Let me try removing the new entrypoint I added and see if that works.

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.

Also please stop forcing pushing it makes it insane to follow changes 😭

Sure. Habit from other projects I contribute to where this is the expected workflow. I just use the "see changes since last revision" button.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants