Repository navigation
Expose getDefaultResultOrder on node:dns/promises and fix its return value - #28949
Conversation
…value
- dns.getDefaultResultOrder() now returns the current order string
("ipv4first" | "ipv6first" | "verbatim") instead of the internal
defaultResultOrder function object.
- Added getDefaultResultOrder and getServers to the dns.promises export
(and therefore to node:dns/promises) to match Node.js.
Fixes #28948
|
Updated 5:21 PM PT - Apr 7th, 2026
❌ @dylan-conway, your commit d50597e has 2 failures in
🧪 To try this PR locally: bunx bun-pr 28949That installs a local version of the PR into your bun-28949 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughExposed Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/28948.test.ts`:
- Around line 2-8: The file header in test/regression/issue/28948.test.ts
contains extra prose about the bug; replace the multi-line comment block with a
single-line GitHub issue URL comment only (leave the one-line issue reference at
the top) and remove the explanatory sentences about node:dns/promises and
getDefaultResultOrder so that the test header contains only the canonical issue
link; move any remaining narrative into the PR/issue description rather than the
test source.
- Around line 15-29: The shared spawn helper run currently asserts exitCode
inside run, which prevents callers from seeing stdout diffs on failure; modify
the run function (the async run(src: string) that spawns Bun via Bun.spawn and
currently returns stdout) to stop asserting exitCode and instead return both
stdout and exitCode (or return [stdout, stderr, exitCode]) so each test can
perform its own assertions; specifically remove the line
expect(exitCode).toBe(0) from run and update callers/tests to assert
expect(stdout).toBe(...) before calling expect(exitCode).toBe(0).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 707f7420-3c5e-4e49-a096-39ffc57c6dc2
📒 Files selected for processing (2)
src/js/node/dns.tstest/regression/issue/28948.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — simple one-liner fix for returning a function instead of a string, plus the missing exports on .
Extended reasoning...
Overview
The PR touches two files: (3 lines changed) and a new regression test file. The fix addresses two bugs: returned the internal function object instead of calling it, and the export was missing and entirely.
Security risks
None. This is a pure Node.js compatibility fix for DNS API surface. No auth, crypto, or permission logic is touched.
Level of scrutiny
Low. The changes are minimal and mechanical: → , and two property additions to an object literal. The correctness is self-evident and verified by the regression tests.
Other factors
The inline bug comments flag two test-quality nits (missing and assertion ordering in the helper). These are legitimate style issues per conventions but do not affect correctness. The underlying fix is sound.
|
✅ No merge conflicts detected when merging into Your branch is good to go! |
1 similar comment
|
✅ No merge conflicts detected when merging into Your branch is good to go! |
There was a problem hiding this comment.
LGTM — all previous feedback addressed, fix is correct and well-tested.
Extended reasoning...
Overview
Two-line fix in src/js/node/dns.ts: calls defaultResultOrder() instead of returning the function reference, and adds getDefaultResultOrder and getServers to the promises export object. A new regression test file covers all three export surfaces (dns, dns.promises, node:dns/promises) with 7 concurrent subprocess tests.
Security risks
None. This is a DNS API surface fix with no auth, crypto, or permission code involved.
Level of scrutiny
Low. The changes are mechanical and narrowly scoped. The bug was a classic "forgot to call the function" typo, and the missing properties on promises are additive. No logic changes, no behavioral changes beyond the fix.
Other factors
All three inline comments from my prior review (test.concurrent, assertion ordering, header cleanup) were addressed in commit 6f30aad. No bugs found by the automated system. The fix matches Node.js behavior as documented in the PR description.
|
CI failures on this build are unrelated flakes:
Neither failure touches |
|
Updated investigation after more test lanes reported: all 12 failing lanes (alpine, debian, ubuntu, darwin-14 × arches) fail identically on |
There was a problem hiding this comment.
LGTM — all prior feedback addressed.
Extended reasoning...
Overview
A 3-line JS-only fix to src/js/node/dns.ts: corrects getDefaultResultOrder() to call the inner function rather than return it, and adds getDefaultResultOrder and getServers to the promises export. A 129-line regression test covers all three export paths and state-sharing behavior.
Security risks
None. This is a pure JavaScript module exposing existing DNS API surface — no auth, crypto, or permission-sensitive code paths involved.
Level of scrutiny
Low. The diff is minimal and mechanical: one missing () and two missing property references. The change mirrors Node.js behavior exactly and the fix is verifiable by reading the existing defaultResultOrder function definition.
Other factors
All feedback from my prior review was addressed in commit 6f30aad: test headers cleaned up, run() now returns {stdout, stderr, exitCode} with stdout asserted before exitCode in each test body, and all 7 tests converted to test.concurrent. CI failures are unrelated infrastructure flakes (Redis/TLS timeouts). The regression test passed 7/7 on the lanes it ran.
|
Gate re-ran on d50597e (merge from main) and flagged as failure. Investigation:
The fix itself is 3 lines in |
|
Build #44321 (on d50597e after main merge): 54 pass / 2 test-lane fails, both identical |
…value (oven-sh#28949) Fixes oven-sh#28948 ## Repro ```js import { promises } from "node:dns"; promises.getDefaultResultOrder(); // TypeError: promises.getDefaultResultOrder is not a function ``` This is exactly what Vite 8's DNS lookup helper does, and it breaks Vite 8 builds under Bun. ## Cause `src/js/node/dns.ts` had two related bugs: 1. `dns.getDefaultResultOrder()` was implemented as `return defaultResultOrder;` — it returned the internal `defaultResultOrder` **function object** instead of invoking it. Node returns a string (`"ipv4first"` / `"ipv6first"` / `"verbatim"`). 2. The `promises` export was missing `getDefaultResultOrder` entirely, and was also missing `getServers`. Both exist on `dns.promises` (and therefore `node:dns/promises`) in Node. ## Fix ```diff function getDefaultResultOrder() { - return defaultResultOrder; + return defaultResultOrder(); } ``` ```diff const promises = { … + getDefaultResultOrder, setDefaultResultOrder, + getServers, setServers, }; ``` `setDefaultResultOrder` mutates module-level state, so `dns`, `dns.promises`, and `node:dns/promises` share the same order value — matching Node's behavior. ## Verification ``` $ bun bd test test/regression/issue/28948.test.ts 7 pass 0 fail 36 expect() calls ``` With the fix stashed, all 7 regression tests fail (returning `"[Function: defaultResultOrder]"` or `undefined`). Existing `test/js/node/dns/node-dns.test.js` suite still passes (66/66). --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
…value (oven-sh#28949) Fixes oven-sh#28948 ## Repro ```js import { promises } from "node:dns"; promises.getDefaultResultOrder(); // TypeError: promises.getDefaultResultOrder is not a function ``` This is exactly what Vite 8's DNS lookup helper does, and it breaks Vite 8 builds under Bun. ## Cause `src/js/node/dns.ts` had two related bugs: 1. `dns.getDefaultResultOrder()` was implemented as `return defaultResultOrder;` — it returned the internal `defaultResultOrder` **function object** instead of invoking it. Node returns a string (`"ipv4first"` / `"ipv6first"` / `"verbatim"`). 2. The `promises` export was missing `getDefaultResultOrder` entirely, and was also missing `getServers`. Both exist on `dns.promises` (and therefore `node:dns/promises`) in Node. ## Fix ```diff function getDefaultResultOrder() { - return defaultResultOrder; + return defaultResultOrder(); } ``` ```diff const promises = { … + getDefaultResultOrder, setDefaultResultOrder, + getServers, setServers, }; ``` `setDefaultResultOrder` mutates module-level state, so `dns`, `dns.promises`, and `node:dns/promises` share the same order value — matching Node's behavior. ## Verification ``` $ bun bd test test/regression/issue/28948.test.ts 7 pass 0 fail 36 expect() calls ``` With the fix stashed, all 7 regression tests fail (returning `"[Function: defaultResultOrder]"` or `undefined`). Existing `test/js/node/dns/node-dns.test.js` suite still passes (66/66). --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
2 participants
Fixes #28948
Repro
This is exactly what Vite 8's DNS lookup helper does, and it breaks Vite 8 builds under Bun.
Cause
src/js/node/dns.tshad two related bugs:dns.getDefaultResultOrder()was implemented asreturn defaultResultOrder;— it returned the internaldefaultResultOrderfunction object instead of invoking it. Node returns a string ("ipv4first"/"ipv6first"/"verbatim").promisesexport was missinggetDefaultResultOrderentirely, and was also missinggetServers. Both exist ondns.promises(and thereforenode:dns/promises) in Node.Fix
function getDefaultResultOrder() { - return defaultResultOrder; + return defaultResultOrder(); }const promises = { … + getDefaultResultOrder, setDefaultResultOrder, + getServers, setServers, };setDefaultResultOrdermutates module-level state, sodns,dns.promises, andnode:dns/promisesshare the same order value — matching Node's behavior.Verification
With the fix stashed, all 7 regression tests fail (returning
"[Function: defaultResultOrder]"orundefined). Existingtest/js/node/dns/node-dns.test.jssuite still passes (66/66).