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

Add support for running using deno #362

Merged
merged 2 commits into from
Oct 10, 2023

Conversation

Douile
Copy link
Contributor

@Douile Douile commented Sep 16, 2023

Prefix node imports with "node:" and gate a socket API that is not implemented in deno so that the library can be used there. This should not break node and doesn't in my brief testing.

@CosminPerRam
Copy link
Member

CosminPerRam commented Sep 16, 2023

Thanks for contributing to Node-GameDig!
Although I'm not sure how many Deno users are for now, I do think (If it doesnt break stuff) that this would be fitting considering the small amount of changes for future use.
I will also test some things before merging.

@CosminPerRam
Copy link
Member

After merging, please make an issue to track denoland/deno#20138 to make the needed changes (hopefully just to remove that if statement) when stuff gets fixed.

bin/gamedig.js Outdated
Comment on lines 71 to 75
.then(() => {
// https://github.com/denoland/deno/issues/20138
if (typeof Deno !== "undefined") {
gamedig.queryRunner.udpSocket?.socket?.close();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont exactly understand something, unref is not implemented, but why do we close the connection only here on the binary one? wouldn't we need to also close instances after being used in lib-only?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sorry I should've mentioned that. I just wasn't sure how to do that on the library as there's no real close method. I guess maybe could close after every query and reconstruct it for the next.

@CosminPerRam
Copy link
Member

Hey, coming back late to this (sorry!), could you rebase please? Also, please add a line to the changelog mentioning support for Deno.

@Douile
Copy link
Contributor Author

Douile commented Oct 9, 2023

93a9095 changed line endings in files, if I rebase I have to recommit entire files. Probably easier to just re-implement.

@CosminPerRam
Copy link
Member

Sorry about that, could you re-implement?

Prefix node imports with "node:" and gate a socket API that is not
implemented in [deno](https://deno.land) so that the library can be used
there. This should not break node and doesn't in my brief testing.
@Douile Douile force-pushed the feat/deno-support branch from dfddd81 to eca552c Compare October 10, 2023 09:22
@CosminPerRam CosminPerRam merged commit 01794f6 into gamedig:master Oct 10, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants