Add support for custom fetch function and/or IANA bootstrap data
#21
Conversation
|
Warning Rate limit exceeded@jakejarvis has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 52 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughAdds a public Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant getRdap as getRdapBaseUrlsForTld()
participant Validate as Validate BootstrapData
participant ResolveFetch as resolveFetch()
participant Fetch as Fetch (withTimeout)
participant Parse as Parse services
Caller->>getRdap: call(tld, options)
alt options.customBootstrapData present
getRdap->>Validate: validate shape (object, services array, [tlds,url[]] tuples)
alt valid
Validate->>Parse: extract & normalize base URLs (case/slash/dedupe)
Parse->>Caller: return base URLs
else invalid
Validate->>Caller: throw descriptive Error
end
else
getRdap->>ResolveFetch: resolve fetch implementation (options.customFetch | global)
ResolveFetch-->>Fetch: fetch(options.customBootstrapUrl || DEFAULT_BOOTSTRAP_URL) with signal/timeout
alt fetch succeeds
Fetch->>Parse: parse services → extract/normalize base URLs
Parse->>Caller: return base URLs
else fetch fails/timeout
Fetch->>Caller: return []
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 0
🧹 Nitpick comments (3)
README.md (1)
120-133: Handle non-OK fetch responses in the caching examples.If the bootstrap endpoint responds with 4xx/5xx, calling
response.json()will throw a confusing error in user code. Showing an explicitresponse.okguard in the README keeps the sample resilient and documents best practices for consumers.Apply this diff in each example before
response.json():const response = await fetch('https://data.iana.org/rdap/dns.json'); + if (!response.ok) { + throw new Error(`Failed to load bootstrap data: ${response.status} ${response.statusText}`); + } const data: BootstrapData = await response.json();src/rdap/bootstrap.ts (1)
35-66: Validate each services entry before iterating.Right now we only check that
servicesis an array. If a user supplies malformed tuples (e.g., missing the URL array), the subsequentsvc[0].mapblows up with aTypeErrorand we lose the nice, actionable error messaging you added. A quick structural check keeps the failure mode consistent and developer-friendly.You can extend the validation block like this:
if (!Array.isArray(data.services)) { throw new Error( 'Invalid customBootstrapData: missing or invalid "services" array. See BootstrapData type for required structure.', ); } + data.services.forEach((svc, idx) => { + if ( + !Array.isArray(svc) || + svc.length < 2 || + !Array.isArray(svc[0]) || + !Array.isArray(svc[1]) + ) { + throw new Error( + `Invalid customBootstrapData: services[${idx}] must be a tuple of [string[], string[]].`, + ); + } + });src/rdap/bootstrap.test.ts (1)
5-7: Restore the original globalfetchafter the suite.Overwriting
global.fetchat module scope leaves the mock in place for every other test file in the run, which can cause hard-to-track leaks once another suite needs the real implementation. Cache the original and restore it inafterAll, or usevi.stubGlobalso Vitest handles cleanup automatically.A minimal fix would be:
-// Mock the fetch function -global.fetch = vi.fn(); +const originalFetch = global.fetch; + +beforeAll(() => { + vi.stubGlobal("fetch", vi.fn()); +}); + +afterAll(() => { + global.fetch = originalFetch; +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(2 hunks)src/rdap/bootstrap.test.ts(1 hunks)src/rdap/bootstrap.ts(1 hunks)src/types.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/rdap/bootstrap.test.ts (2)
src/types.ts (1)
BootstrapData(200-213)src/rdap/bootstrap.ts (1)
getRdapBaseUrlsForTld(20-72)
src/rdap/bootstrap.ts (3)
src/types.ts (2)
LookupOptions(239-290)BootstrapData(200-213)src/lib/async.ts (1)
withTimeout(1-17)src/lib/constants.ts (1)
DEFAULT_TIMEOUT_MS(1-1)
jakejarvis
changed the title
fetch function and/or IANA bootstrap data
jakejarvis
force-pushed
the
feat/custom-bootstrap-data
branch
from
7f1c346 to
d38681f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/rdap/bootstrap.ts (1)
6-6: Excellent priority-driven bootstrap resolution with thorough validation!The implementation clearly follows the documented priority order (customBootstrapData → customBootstrapUrl → default IANA URL) and includes comprehensive validation with helpful error messages. The use of
resolveFetchproperly enables custom fetch implementations, and the fallback to an empty array on fetch failure is appropriate.Optional: Consider treating explicit
undefinedthe same as missing optionThe current check
"customBootstrapData" in options(line 28) will throw an error if a user passes{ customBootstrapData: undefined }explicitly, rather than falling back to fetching. While this is caught by validation with a clear error message, it might be more user-friendly to treatundefinedthe same as a missing option.For example, a user with optional caching might write:
const cached = await getFromCache(); // returns BootstrapData | undefined await lookup('example.com', { customBootstrapData: cached });Consider using:
-if (options && "customBootstrapData" in options) { +if (options?.customBootstrapData !== undefined) { data = options.customBootstrapData;This would allow
undefinedto fall through to the fetch logic naturally. However, this is a minor edge case and the current behavior is defensible.Also applies to: 11-19, 25-69
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.md(2 hunks)src/lib/fetch.test.ts(1 hunks)src/lib/fetch.ts(1 hunks)src/rdap/bootstrap.test.ts(1 hunks)src/rdap/bootstrap.ts(1 hunks)src/rdap/client.ts(2 hunks)src/rdap/merge.ts(2 hunks)src/types.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/rdap/merge.ts
- src/lib/fetch.test.ts
- src/rdap/bootstrap.test.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/lib/fetch.ts (1)
src/types.ts (1)
FetchLike(375-378)
src/rdap/bootstrap.ts (4)
src/types.ts (2)
LookupOptions(239-339)BootstrapData(200-213)src/lib/fetch.ts (1)
resolveFetch(26-28)src/lib/async.ts (1)
withTimeout(1-17)src/lib/constants.ts (1)
DEFAULT_TIMEOUT_MS(1-1)
src/rdap/client.ts (2)
src/lib/fetch.ts (1)
resolveFetch(26-28)src/lib/async.ts (1)
withTimeout(1-17)
🪛 LanguageTool
README.md
[style] ~419-~419: Consider replacing ‘only’ with a different word to let your writing stand out.
Context: ...t be fetched, so your custom fetch will only be used for RDAP domain and entity/rela...
(ONLY_EXCLUSIVELY_STYLE)
🔇 Additional comments (4)
src/types.ts (1)
1-378: Excellent public API documentation!The comprehensive JSDoc comments and code examples significantly improve the developer experience. The type definitions are well-structured and the new
BootstrapData,LookupResult, andFetchLiketypes provide clear contracts for the new functionality.src/rdap/client.ts (1)
3-3: Clean integration of fetch resolution!The changes properly wire in the
resolveFetchutility while maintaining existing timeout behavior viawithTimeout. The dependency injection pattern enables custom fetch implementations without disrupting the existing logic.Also applies to: 19-21
src/lib/fetch.ts (1)
1-28: Well-designed utility function!The
resolveFetchutility provides a clean, centralized way to resolve fetch implementations across all RDAP HTTP operations. The comprehensive JSDoc documentation with usage examples makes the intent and usage clear.README.md (1)
89-420: Outstanding documentation with practical examples!The new documentation sections for bootstrap data caching and custom fetch implementation are comprehensive and user-friendly. The multiple examples covering different use cases (in-memory, Redis, filesystem, retry logic, etc.) make it easy for developers to implement these features in their specific contexts. The explanations of why these features are useful provide excellent motivation.
Also applies to: 431-433
…ookup function; enhance error handling in lookup and normalize functions
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/dates.ts (1)
101-101: Good defensive guards.These checks prevent invalid Date construction when regex captures are unexpectedly missing. The guards correctly validate the destructured variables before usage, improving robustness.
For consistency, consider adding similar guards to the time-based parsing path:
if (m[0].includes(":")) { const [_, y, mo, d, hh, mm, ss, offH, offM] = m; + if (!y || !mo || !d || !hh || !mm || !ss) return undefined; // Base time as UTC let dt = Date.UTC(Also applies to: 113-113
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
package.json(1 hunks)src/index.smoke.test.ts(3 hunks)src/index.test.ts(5 hunks)src/index.ts(1 hunks)src/lib/dates.ts(2 hunks)src/lib/text.ts(1 hunks)src/rdap/bootstrap.ts(1 hunks)src/rdap/normalize.test.ts(1 hunks)src/types.ts(5 hunks)src/whois/merge.test.ts(1 hunks)src/whois/normalize.test.ts(1 hunks)src/whois/normalize.ts(2 hunks)src/whois/referral.test.ts(1 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/index.test.ts (1)
src/index.ts (1)
lookup(23-122)
src/index.smoke.test.ts (1)
src/index.ts (3)
lookup(23-122)isRegistered(141-148)isAvailable(128-135)
src/rdap/bootstrap.ts (4)
src/types.ts (2)
LookupOptions(239-339)BootstrapData(200-213)src/lib/fetch.ts (1)
resolveFetch(26-28)src/lib/async.ts (1)
withTimeout(1-17)src/lib/constants.ts (1)
DEFAULT_TIMEOUT_MS(1-1)
🔇 Additional comments (16)
package.json (1)
53-53: LGTM: Type definitions patch update.The
@types/nodeupdate from 24.9.0 to 24.9.2 is a standard patch version bump that keeps Node.js type definitions current without introducing breaking changes.src/whois/merge.test.ts (1)
32-33: Good defensive test guard.The runtime check ensures the test fails explicitly with a clear message if the chain is unexpectedly empty, rather than failing obscurely on the destructuring or subsequent assertions.
src/lib/text.ts (1)
15-15: LGTM: Stricter bracket parsing guard.The updated condition ensures both capture groups are defined before processing the bracketed form, preventing edge cases where the regex match succeeds but groups are unexpectedly undefined.
src/whois/normalize.test.ts (1)
14-15: LGTM: Defensive test assertions.The two-step assertion pattern (first checking array existence, then accessing nested properties with optional chaining) makes test failures more specific and aligns with similar updates across test files.
src/rdap/normalize.test.ts (1)
71-72: LGTM: Consistent defensive test pattern.The two-step assertion matches the pattern introduced in
src/whois/normalize.test.ts, ensuring consistent test practices across the codebase.src/whois/normalize.ts (2)
158-164: LGTM: Stricter status parsing with null filtering.The updated logic correctly filters out status lines where the first token is empty or undefined, ensuring only valid status objects are included. The type guard provides proper type narrowing for TypeScript.
227-229: LGTM: Defensive transfer lock check.The additional guard ensures
s.statusexists before applying the regex, preventing potential runtime issues with missing status fields. This aligns with the stricter type checking enabled intsconfig.json(noUncheckedIndexedAccess).tsconfig.json (1)
4-23: LGTM: Enhanced type safety and modern target.The compiler configuration updates introduce several beneficial changes:
- ES2022 target: Modern JavaScript features with good runtime support
- Stricter type checking:
noUncheckedIndexedAccess,noUnusedParameters,noFallthroughCasesInSwitch, andforceConsistentCasingInFileNameshelp catch bugs at compile time- Better debugging:
declarationMapandsourceMapenable source-level debuggingThe
noUncheckedIndexedAccessoption is particularly strict and explains the defensive coding patterns introduced throughout this PR (e.g., explicit undefined checks innormalize.tsand tests).src/types.ts (4)
179-213: ExcellentBootstrapDatainterface design.The interface accurately models the IANA RDAP bootstrap JSON structure with:
- Clear documentation and JSON example
- Proper RFC 7484 reference
- Type-safe services array structure
This enables users to implement their own caching strategies for bootstrap data.
256-332: Outstanding documentation forcustomFetchoption.The documentation provides:
- Clear explanation of the feature's purpose
- Multiple practical use cases (caching, logging, retry, rate limiting, proxies, testing)
- Two concrete code examples
- Proper linkage to the
FetchLiketypeThis empowers users to implement sophisticated HTTP request handling strategies.
1-367: Well-structured public API expansion.The type definitions introduce a clean, well-documented public API with:
- Comprehensive JSDoc comments throughout
- Practical code examples
- Clear relationships between types
- Appropriate granularity (RegistrarInfo, Contact, Nameserver, StatusEvent)
The additions enable advanced use cases (custom bootstrap data, custom fetch implementations) while maintaining backward compatibility.
375-378: FetchLike type narrowing is correct and intentional—no changes needed.Verification confirms the narrowing from
RequestInfo | URLtostring | URLis safe and consistent with actual usage:
- No code instantiates or passes
Requestobjects to custom fetch functions- All test invocations use string URLs with
initcontaining only standard properties (method,signal)- Documentation examples match the implementation
- The change aligns with the actual API contract
The concern raised in the original review comment does not reflect the codebase's actual usage patterns.
src/whois/referral.test.ts (1)
43-44: Safer chain assertion acknowledgedThanks for guarding the optional referral entry while keeping the assertion intact.
src/index.ts (1)
113-115: Good short-circuit for missing WHOIS dataAppreciate the explicit failure response when normalization produces nothing; this avoids merging undefined records and surfaces a clear error.
src/index.test.ts (1)
84-110: Test coverage still exercises full orchestrationNice job updating the orchestration tests to call
lookupdirectly; this keeps the original scenarios intact while covering the renamed public entry point.src/index.smoke.test.ts (1)
7-21: Conditional smoke harness works wellUsing
maybeTestkeeps the smoke suite available locally while skipping by default in CI—sensible toggle.
… minute, and second components in parseDateWithRegex function
…-catch block to manage network, timeout, and JSON parse errors, while preserving cancellation behavior.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests