-
Notifications
You must be signed in to change notification settings - Fork 95
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
feat: customize ipns dnsResolvers #445
Merged
Merged
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
77c127a
feat(verified-fetch): customize ipns dnsResolvers
SgtPooki 29bf8fc
chore: pr comment
SgtPooki 5efdb95
Merge branch 'main' into feat/customize-dns-resolvers
SgtPooki fcc5c32
test: fix dns-resolvers test
SgtPooki fc1e5b8
chore: harden custom dns resolver test
SgtPooki 348e483
test: actually fix the custom dns resolver test
SgtPooki f180047
chore: firefox promise.any aggregate name is different
SgtPooki 8f3459e
fix: createVerifiedFetch passes through custom dnsResolvers
SgtPooki cd9a008
docs: jsdoc fix from pr comment
SgtPooki 69a025f
Merge branch 'main' into feat/customize-dns-resolvers
SgtPooki a7b5b89
test: move custom-dns-resolvers tests
SgtPooki 07a4ad4
test: move content-type-parser test
SgtPooki 17fdb48
docs: typo
SgtPooki cca3841
fix: dnsResolvers is passed in first arg to createVerifiedFetch
SgtPooki File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
import { stop } from '@libp2p/interface' | ||
import { expect } from 'aegir/chai' | ||
import Sinon from 'sinon' | ||
import { createVerifiedFetch } from '../src/index.js' | ||
import { VerifiedFetch } from '../src/verified-fetch.js' | ||
import { createHelia } from './fixtures/create-offline-helia.js' | ||
import type { Helia } from '@helia/interface' | ||
|
||
describe('custom dns-resolvers', () => { | ||
let helia: Helia | ||
|
||
beforeEach(async () => { | ||
helia = await createHelia() | ||
}) | ||
|
||
afterEach(async () => { | ||
await stop(helia) | ||
}) | ||
|
||
it('is used when passed to createVerifiedFetch', async () => { | ||
const customDnsResolver = Sinon.stub() | ||
|
||
customDnsResolver.returns(Promise.resolve('/ipfs/QmVP2ip92jQuMDezVSzQBWDqWFbp9nyCHNQSiciRauPLDg')) | ||
|
||
const fetch = await createVerifiedFetch(helia, { | ||
dnsResolvers: [customDnsResolver] | ||
}) | ||
// error of walking the CID/dag because we haven't actually added the block to the blockstore | ||
await expect(fetch('ipns://some-non-cached-domain.com')).to.eventually.be.rejected.with.property('errors').that.has.lengthOf(0) | ||
|
||
expect(customDnsResolver.callCount).to.equal(1) | ||
expect(customDnsResolver.getCall(0).args).to.deep.equal(['some-non-cached-domain.com', { onProgress: undefined }]) | ||
}) | ||
|
||
it('is used when passed to VerifiedFetch', async () => { | ||
const customDnsResolver = Sinon.stub() | ||
|
||
customDnsResolver.returns(Promise.resolve('/ipfs/QmVP2ip92jQuMDezVSzQBWDqWFbp9nyCHNQSiciRauPLDg')) | ||
|
||
const verifiedFetch = new VerifiedFetch({ | ||
helia | ||
}, { | ||
dnsResolvers: [customDnsResolver] | ||
}) | ||
// error of walking the CID/dag because we haven't actually added the block to the blockstore | ||
await expect(verifiedFetch.fetch('ipns://some-non-cached-domain2.com')).to.eventually.be.rejected.with.property('errors').that.has.lengthOf(0) | ||
|
||
expect(customDnsResolver.callCount).to.equal(1) | ||
expect(customDnsResolver.getCall(0).args).to.deep.equal(['some-non-cached-domain2.com', { onProgress: undefined }]) | ||
}) | ||
}) |
Oops, something went wrong.
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.
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.
Another question here. Do we want to add dnsResolvers to the first object?
to stick to the pattern of "components in first object, options that alter how things work in the second" I think it would make sense in either place.. 🤔
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.
I think
dnsResolvers
are deserving of the same importance asrouters
which makes the case to move it to the first object.routers
is currently used for IPNS routing (and CID routing in the future). Assuming that we see IPNS and DNSLink as being of equal importancednsResolvers
should live besiderouters
in my humble opinion.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.
On the other hand, customising dnsResolvers requires importing the constructors for dnsOverHttps and dnsJsonOverHttps which does reduce ergonomics.
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'll still be optional either way, but I think I agree with you.
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.
I think maybe in the first argument, keeping the routers/gateways/resolvers together makes sense to me.
The only thing we have in the second arg is
contentTypeParser
, not 100% on why we have two args right now anyway!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.
After looking at this for a bit and starting to change both args to a single object, but contentTypeParser has to be in the second obj, or we need to accept a possible
.helia
option in the first obj, or else users can't pass both a custom Helia & contentTypeParser