Use dns.lookup and dns.resolve4 not just dns.resolve4 alone#102
Merged
Conversation
Code Review by Qodo
1. Dist build not updated
|
Up to standards ✅🟢 Issues
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+273
to
+290
| const [A, AAAA, CNAME] = await Promise.all([ | ||
| dns.resolve4(host).catch(() => [] as string[]), | ||
| dns.resolve6(host).catch(() => [] as string[]), | ||
| dns.resolveCname(host).catch(() => [] as string[]) | ||
| ]); | ||
|
|
||
| if (A.length === 0 && AAAA.length === 0) { | ||
| try { | ||
| const lookups = await dns.lookup(host, { all: true }); | ||
| for (const entry of lookups) { | ||
| if (entry.family === 4) A.push(entry.address); | ||
| if (entry.family === 6) AAAA.push(entry.address); | ||
| } | ||
| } catch {} | ||
| } | ||
|
|
||
| return { A, AAAA, CNAME }; | ||
| } |
There was a problem hiding this comment.
1. Dist build not updated 🐞 Bug ≡ Correctness
The PR updates resolve_all_records() in src/helpers.ts, but the runtime entrypoint is dist/utils.js which uses dist/helpers.js; since dist/helpers.js still contains the old resolver implementation, the shipped behavior will not include the new dns.lookup fallback.
Agent Prompt
### Issue description
The package runtime entrypoint is `dist/utils.js` (CommonJS), which pulls in `dist/helpers.js`. This PR changes DNS behavior only in `src/helpers.ts`, leaving the compiled `dist/` artifacts stale. As a result, consumers will not receive the new `dns.lookup` fallback.
### Issue Context
- `package.json` declares `main: dist/utils.js` and exports `./dist/utils.js`.
- `dist/helpers.js` still contains the pre-change `resolve_all_records()` implementation.
- `dist/` is not ignored, and there is no `prepublishOnly`/`prepare` script shown that would automatically rebuild `dist/` on publish.
### Fix Focus Areas
- src/helpers.ts[270-290]
- dist/helpers.js[253-263]
- package.json[20-36]
- .gitignore[1-2]
### Suggested fix
1. Run `npm run build` (tsc) to regenerate `dist/helpers.js`, `dist/utils.js`, and the corresponding `.d.ts` outputs.
2. Commit the updated `dist/` artifacts.
3. (Optional but recommended) Add a `prepublishOnly` (or `prepare`) script to ensure `dist/` is always rebuilt before publishing.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Summary by Qodo
Fallback to dns.lookup when resolve4/resolve6 return no A/AAAA records
🐞 Bug fix✨ Enhancement🕐 10-20 MinutesWalkthroughs
Description
Diagram
graph TD A["is_url_safe()"] --> B["is_hostname_resolve_to_internal_ip()"] --> C["resolve_all_records()"] C --> D{{"dns.resolve4/6/cname"}} --> E{"A/AAAA empty?"} E -->|"yes"| F{{"dns.lookup(all:true)"}} --> G["Merge A/AAAA"] --> H["Return records"] E -->|"no"| H subgraph Legend direction LR _fn["Function"] ~~~ _ext{{"Node.js DNS"}} ~~~ _dec{"Decision"} endHigh-Level Assessment
The following are alternative approaches to this PR:
1. Prefer dns.lookup first, then record-specific resolves
2. Use dns.resolveAny as a single query path
3. Add caching for resolution results (short TTL)
Recommendation: The PR’s approach (parallel resolve4/6/cname plus a lookup-based fallback only when A/AAAA are empty) is a good balance: it keeps explicit DNS-record queries as the primary source while addressing real-world cases where lookup succeeds but resolve4/6 yields no addresses. If this code runs frequently, consider a small TTL cache later, but it’s not required for correctness.
File Changes
Bug fix (1)