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.
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
feat: customize ipns dnsResolvers #445
Changes from 9 commits
77c127a
29bf8fc
5efdb95
fcc5c32
fc1e5b8
348e483
f180047
8f3459e
cd9a008
69a025f
a7b5b89
07a4ad4
17fdb48
cca3841
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 & contentTypeParserThere 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.
this aggregate error should probably have a length greater than zero 🤔
Looks like we're logging the error, but not returning them:
helia/packages/verified-fetch/src/verified-fetch.ts
Line 314 in 7c07e11
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.
FWIW,
AggregateError
has really poor DX as the default message logged to the console doesn't include any of the actual errors which can lead to spurious issues being reported and a lot of wasted time.In other places where there's only one entry in
.errors
we unwind it to just throw that error instead.In this case, if there's an error, it should be made available, otherwise no error should be thrown?