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(dns): initial support dns module #825

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

nabetti1720
Copy link
Contributor

@nabetti1720 nabetti1720 commented Feb 9, 2025

Issue # (if available)

closed #827

Description of changes

This PR is an initial implementation of a Node.js compatible dns module. However, like other modules, we do not aim to implement all functions.

  • The most frequently used function, dns.lookup(), has not been implemented.
  • The implementation of options is also limited. Currently, only the family of numeric or record types is valid.

Checklist

  • Created unit tests in tests/unit and/or in Rust for my feature if needed
  • Ran make fix to format JS and apply Clippy auto fixes
  • Made sure my code didn't add any additional warnings: make check
  • Added relevant type info in types/ directory
  • Updated documentation if needed (API.md/README.md/Other)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@Sytten
Copy link
Collaborator

Sytten commented Feb 9, 2025

This should reuse the DNS resolver we added for llrt_http. Probably need to split a lib crate.

@nabetti1720 nabetti1720 marked this pull request as draft February 9, 2025 23:27
@nabetti1720 nabetti1720 force-pushed the feat/support-dns-module branch from 00e4b78 to c908668 Compare February 11, 2025 01:40
Copy link
Collaborator

@richarddavison richarddavison left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Some comments!

@nabetti1720
Copy link
Contributor Author

Thanks for the PR. Some comments!

@richarddavison , Thank you for your comment.
I've been busy with my day job and have been away from this project for a while, but it looks like I'm finally going to be able to start working on it little by little.

Please be patient and wait for me. :)

@nabetti1720 nabetti1720 marked this pull request as ready for review February 23, 2025 08:22
@nabetti1720
Copy link
Contributor Author

nabetti1720 commented Feb 23, 2025

Currently blocked by #847. I intend to rebase this branch after #847 is merged.

@nabetti1720 nabetti1720 force-pushed the feat/support-dns-module branch from c756460 to 159b58c Compare February 23, 2025 22:11
Copy link
Collaborator

@richarddavison richarddavison left a comment

Choose a reason for hiding this comment

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

A minor suggestion and we should be good to merge. Thanks for this!

@nabetti1720
Copy link
Contributor Author

nabetti1720 commented Feb 24, 2025

A minor suggestion and we should be good to merge. Thanks for this!

Sorry if I overlooked your suggestion. Is there still something to improve?

Comment on lines +3 to +27
describe("lookup", () => {
it("optionless", (done) => {
dns.lookup("localhost", (err, address, family) => {
expect(address === "::1" || address === "127.0.0.1").toBeTruthy();
expect(family === 4 || family === 6).toBeTruthy();
done();
});
});

it("option - integer", (done) => {
dns.lookup("localhost", 4, (err, address, family) => {
expect(address).toEqual("127.0.0.1");
expect(family).toEqual(4);
done();
});
});

it("option - record", (done) => {
dns.lookup("localhost", { family: 4 }, (err, address, family) => {
expect(address).toEqual("127.0.0.1");
expect(family).toEqual(4);
done();
});
});
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add error test

@richarddavison
Copy link
Collaborator

A minor suggestion and we should be good to merge. Thanks for this!

Sorry if I overlooked your suggestion. Is there still something to improve?

My misstake. For some reason the requested change was not published

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.

Integration of DNS resolver functions
3 participants