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

fix: provide nodejs compat for dns methods wrapped with util.promisify #6748

Merged
merged 9 commits into from
Oct 31, 2023

Conversation

antongolub
Copy link
Contributor

@antongolub antongolub commented Oct 27, 2023

What does this PR do?

Adds nodejs compatibility for dns[methods] wrapped with util.promisify

nodejs

> var u = require('util'), d = require('dns');
undefined
> await u.promisify(d.lookup)('examle.com')
{ address: '45.33.20.235', family: 4 }

bun

> var u = require('util'), d = require('dns')
undefined
> await u.promisify(d.lookup)('examle.com')
'45.33.20.235'

How did you verify your code works?

> var u = require('util'), d = require('dns')
undefined
> await u.promisify(d.lookup)('examle.com')
'45.33.20.235'
> d.lookup[u.promisify.custom] = d.promises.lookup
[Function: lookup]
> await u.promisify(d.lookup)('examle.com')
{ address: '45.33.20.235', family: 4 }

UPD

bun/build/bun-debug test js/node/dns

@antongolub antongolub changed the title fix: add nodejs compat for dns methods wrapping with util.promisify fix: provide nodejs compat for dns methods wrapping with util.promisify Oct 27, 2023
@antongolub antongolub changed the title fix: provide nodejs compat for dns methods wrapping with util.promisify fix: provide nodejs compat for dns methods wrapped with util.promisify Oct 27, 2023
Copy link
Collaborator

@Jarred-Sumner Jarred-Sumner left a comment

Choose a reason for hiding this comment

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

Thank you for this

One small nitpicky comment but lets merge after that

@@ -1,6 +1,7 @@
// Hardcoded module "node:dns"
// only resolve4, resolve, lookup, resolve6, resolveSrv, and reverse are implemented.
const dns = Bun.dns;
const util = require("node:util");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of loading all of node:util, we can choose to instead load only the symbol we need:

Suggested change
const util = require("node:util");
const utilPromisifyCustomSymbol = Symbol.for("nodejs.util.promisify.custom")

Copy link
Contributor Author

@antongolub antongolub Oct 30, 2023

Choose a reason for hiding this comment

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

Hmm...
It turns out, there's a pair of symbols: nodejs.util.promisify.custom and util.promisify.custom

https://github.com/oven-sh/bun/blob/main/src/js/node/util.js#L150

Copy link
Contributor Author

@antongolub antongolub Oct 30, 2023

Choose a reason for hiding this comment

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

// TODO: register shared symbol promisify.custom — is it time?

[resolveTxt, promises.resolveTxt],
[resolveNaptr, promises.resolveNaptr],
]) {
method[util.promisify.custom] = pMethod
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
method[util.promisify.custom] = pMethod
method[utilPromisifyCustomSymbol] = pMethod

Copy link
Contributor Author

@antongolub antongolub Oct 30, 2023

Choose a reason for hiding this comment

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

Finally, it works on my machine

✓ uses `dns.promises` implementations for `util.promisify` factory > resolvePtr
✓ uses `dns.promises` implementations for `util.promisify` factory > resolveSoa
✓ uses `dns.promises` implementations for `util.promisify` factory > resolveSrv
✓ uses `dns.promises` implementations for `util.promisify` factory > resolveTxt
✓ uses `dns.promises` implementations for `util.promisify` factory > resolveNaptr
[Loop] ref
[FilePoll] register: machport (9735)
[Loop] unref x 1
[FilePoll] machport
[FilePoll] onUpdate kevent (fd: 2147483646) GetAddrInfoRequest
[GetAddrInfoRequest] getAddrInfoAsyncCallback: status=0
[FilePoll] unregister: machport (2147483646) skipped due to needs_rearm
[Loop] ref
[FilePoll] register: machport (9739)
[Loop] unref x 1
[FilePoll] machport
[FilePoll] onUpdate kevent (fd: 2147483646) GetAddrInfoRequest
[GetAddrInfoRequest] getAddrInfoAsyncCallback: status=0
[FilePoll] unregister: machport (2147483646) skipped due to needs_rearm
✓ uses `dns.promises` implementations for `util.promisify` factory > util.promisify(dns.lookup) acts like dns.promises.lookup [4.13ms]

Some docs are referencing to make js, which seems useless. The required files are missing.

src/js/_codegen/index.ts
src/js/build-esm.ts

@Jarred-Sumner Jarred-Sumner merged commit 732650d into oven-sh:main Oct 31, 2023
@Jarred-Sumner
Copy link
Collaborator

Thank you!

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.

None yet

2 participants