-
Notifications
You must be signed in to change notification settings - Fork 134
feat!: use custom DNS resolver in @helia/ipns for DNSLink #466
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
Conversation
Exposes a `.dns` property on the Helia interface for use with other modules such as @helia/ipns. Refs: ipfs/helia-verified-fetch#13 (comment)
Uses the `.dns` property from #465 to resolve DNS `TXT` records. This allows configuring discrete resolvers for different TLDs, unifies caching across different use of DNS (e.g. dnsaddr multiaddrs), etc. BREAKING CHANGE: requires @helia/[email protected] or later, `resolveDns` has been renamed `resolveDNSLink`
|
#465 should be merged first to prevent this change causing a breaking change to cascade to other modules in this monorepo. |
| result = result.substring(1, result.length - 1) | ||
| } | ||
|
|
||
| if (!result.startsWith('dnslink=')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we do
| if (!result.startsWith('dnslink=')) { | |
| if (!result.startsWith('dnslink=/ip')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because we want to handle weird things like dnslink=/dnslink/blah.com
| if (result.startsWith('"') && result.endsWith('"')) { | ||
| result = result.substring(1, result.length - 1) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if there is a leading or trailing " will there always be the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empirical evidence suggests yes, the spec is silent.
| * @default 32 | ||
| */ | ||
| resolvers?: DNSResolver[] | ||
| maxRecursiveDepth?: number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the method for users to pass a custom one-off resolver with these new changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add an optional dns?: DNS key, but the caching situation gets a little weird - is this something we do often?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was requested by a user a few months ago, but a dnsResolver map might handle their usecase?
Uses the
.dnsproperty from #465 to resolve DNSTXTrecords.This allows configuring discrete resolvers for different TLDs, unifies caching across different use of DNS (e.g. dnsaddr multiaddrs), etc.
Refs: ipfs/helia-verified-fetch#13 (comment)
Fixes: #369
BREAKING CHANGE: requires @helia/[email protected] or later,
resolveDnshas been renamedresolveDNSLinkChange checklist