implement node:dns module for nodejs_compat #3183
Conversation
anonrig
force-pushed
the
yagiz/implement-dns-nodejs
branch
from
171e277
to
19f1629
Compare
| throw new DnsError(name, errorCodes.BADRESP, syscall); | ||
| } | ||
|
|
||
| if (json.Answer?.at(0).name === '') { |
There was a problem hiding this comment.
| if (json.Answer?.at(0).name === '') { | |
| if (json.Answer?.[0]?.name === '') { |
at -> [] is more concise
?.name not to throw if the array is empty
There was a problem hiding this comment.
On typescript [0] doesn't return undefined | ValueType but .at() does. I think we should encourage it more. And we should also enable noUncheckedIndexedAccess on tsconfig as well (ref: https://www.typescriptlang.org/tsconfig/#noUncheckedIndexedAccess)
There was a problem hiding this comment.
On typescript [0] doesn't return undefined | ValueType but .at() does.
Did you mean to say "does not"?
If that's the case TS is lying to you:
| // hostname: 'google.com2' | ||
| // } | ||
| export class DnsError extends NodeError { | ||
| errno: undefined = undefined; |
There was a problem hiding this comment.
Would the following work?
| errno: undefined = undefined; | |
| errno = undefined; |
|
|
||
| constructor(hostname: string, code: string, syscall: string) { | ||
| super(code, `${syscall} ${code} ${hostname}`); | ||
| this.errno = undefined; |
| 'One of the entries should have spf1' | ||
| ); | ||
| ok( | ||
| results.at(0).at(0)[0] !== '"', |
There was a problem hiding this comment.
| results.at(0).at(0)[0] !== '"', | |
| results[0][0][0] !== '"', |
There was a problem hiding this comment.
So I guess it should be results[0]?.[0]?.[0]
| // Callback API | ||
| const { promise, resolve, reject } = Promise.withResolvers(); | ||
| dns.resolveTxt('cloudflare.com', (error, results) => { | ||
| if (error) { | ||
| reject(error); | ||
| return; | ||
| } | ||
| testResults(results); | ||
| resolve(); | ||
| }); | ||
| await promise; |
There was a problem hiding this comment.
| // Callback API | |
| const { promise, resolve, reject } = Promise.withResolvers(); | |
| dns.resolveTxt('cloudflare.com', (error, results) => { | |
| if (error) { | |
| reject(error); | |
| return; | |
| } | |
| testResults(results); | |
| resolve(); | |
| }); | |
| await promise; | |
| // Callback API | |
| dns.resolveTxt('cloudflare.com', (error, results) => { | |
| if (error) { | |
| return fail(error); | |
| } | |
| testResults(results); | |
| }); |
| setServers, | ||
| } from 'node-internal:internal_dns'; | ||
|
|
||
| function toCallback<A = unknown, ReturnValue = unknown>( |
There was a problem hiding this comment.
You know we implement callbackify? You can move its implementation into node/internal and reuse that here.
| dnsUtil.getDefaultResultOrder.bind(dnsUtil); | ||
| export const setServers = dnsUtil.setServers.bind(dnsUtil); | ||
| export const reverse = toCallback(dns.reverse.bind(dns)); | ||
| export const resolveTxt = toCallback(dns.resolveTxt.bind(dns)); |
There was a problem hiding this comment.
Since you're going from promises to callbacks here, how are you handling synchronously throwing on argument validation? By default the JSG wrapper should cause synchronously thrown errors at the c++ level to be returned into promise rejections. Are you certain this will do the right thing for the errors the Node.js API expects to be thrown synchronously?
There was a problem hiding this comment.
I'll remove async from functions and just return a promise from the function, which would throw validation errors synchronously but the function will return Promise
| ) => void { | ||
| return ( | ||
| input: A, | ||
| callback: (error: Error | null, value?: ReturnValue) => void |
There was a problem hiding this comment.
Node.js will throw an error on these if callback is not a function, correct?
There was a problem hiding this comment.
Yes, definitely there is a validateFunction missing from here.
|
|
||
| let json: SuccessResponse | FailedResponse; | ||
| try { | ||
| const response = await fetch(server, { |
There was a problem hiding this comment.
Just noting... it will be important to document to folks that DNS queries will count towards the subrequest limits and concurrent subrequest limits will apply.
There was a problem hiding this comment.
Is there an internal API to remove this sub request limit from this call?
| ): Promise<SuccessResponse> { | ||
| // We are using cloudflare-dns.com and not 1.1.1.1 because of certificate issues. | ||
| // TODO(soon): Replace this when KJ certificate issues are resolved. | ||
| const server = new URL('https://cloudflare-dns.com/dns-query'); |
There was a problem hiding this comment.
Since this will route these requests as public internet requests I guess we should decide if using global fetch is really what we want vs. using a private fetch via a binding that would for sure route the request through internal infrastructure.
There was a problem hiding this comment.
Yes, for sure, we should find a solution to this before landing this pull-request.
Implements all functions in
node:dnsusing Cloudflare DNS (1.1.1.1)