feat: support p2p retrieval by default #130
Conversation
|
As discussed during Helia WG: Moving forward suggestion we agreed on: |
2color
changed the title
|
@2color I think we need to work on updating the deps across all packages and getting gateway conformance passing before this will pass. I'm focusing on updating the deps, and once that is done, we should be able to pass this one. |
packages/verified-fetch/src/index.ts
Outdated
| ], | ||
| dns: createDns(init?.dnsResolvers), |
There was a problem hiding this comment.
FYI: setting libp2pConfig.dns was causing some failures in tests. do we need that? dns should be handled at the helia layer shouldn't it?
There was a problem hiding this comment.
I think both Helia and libp2p rely on the DNS type. For libp2p it's mostly for resolving dnsaddr multiaddrs.
|
NOTE: chrome browser tests are failing in CI because playwright isn't installed. I pinged IPDX on slack |
They're failing because https://github.com/microsoft/playwright shipped a breaking change - the fix is here: hugomrdias/playwright-test#685 |
|
The browser tests should run now but 74e27ac broke ts compilation so that'll need to be fixed first. |
This reverts commit 74e27ac.
|
I realised that I didn't consider node.js in this PR. So some of the libp2p config modifications don't make sense. For example, using a circuit relay listener in node.js is helpful for hole punching but isn't necessary in browsers. Same thing with the DHT client. This is a bit of a hairy one, but I'm thinking that we may just want to have a browser override in this package and handle it like we do in Helia. |
and remove kad-dht
There was a problem hiding this comment.
I appreciate that you were able to fix this, but I'm worried that this is appeasing the TS compiler at the cost of the types actually helping us.
There was a problem hiding this comment.
The thing with the services type is that we don't really care since we don't ever touch them.. but I mostly just wanted to unblock the build. I am working on adding a test now.
sometimes, you have to coerce types, and I think the pros of coercing types outweigh the cons of adding a bunch of direct deps on all the libp2p things
| export interface Libp2pServices extends BaseServiceMap, DelegatedRoutingServices, Record<string, any> { | ||
| // eslint-disable-next-line @typescript-eslint/consistent-indexed-object-style | ||
| export interface Libp2pServices extends BaseServiceMap, DelegatedRoutingServices { | ||
| [key: string]: any |
There was a problem hiding this comment.
This is a bit weird, Record<string, any> and { [key: string]: any } are the same type. How does the Record version break things?
There was a problem hiding this comment.
[12:42:33] tsc [started]
src/index.ts(823,33): error TS2345: Argument of type 'VerifiedFetchLibp2p' is not assignable to parameter of type 'Libp2pOptions<{ [x: string]: ...; }> | undefined'.
Type 'VerifiedFetchLibp2p' is not assignable to type 'Libp2pOptions<{ [x: string]: ...; }>'.
Type 'VerifiedFetchLibp2p' is not assignable to type 'Libp2pInit<{ [x: string]: ...; }>'.
Types of property 'services' are incompatible.
Type 'Libp2pServices' is not assignable to type 'ServiceFactoryMap<{ [x: string]: ...; }>'.
'string' index signatures are incompatible.
Type 'unknown' is not assignable to type '(components: Components & { [x: string]: ...; }) => unknown'.
[12:42:34] tsc [failed]
[12:42:34] → Command failed with exit code 1: tsc
There was a problem hiding this comment.
We can do something like
export type Libp2pServices<T extends ServiceMap = ServiceMap> = {
[Property in keyof T]: (components: any & T) => T[Property]
} & BaseServiceMap & DelegatedRoutingServices
¯\_(ツ)_/¯
|
There are so many fixes here that have nothing to do with libp2p or enabling p2p by default.. these changes should probably be pulled out into #141 |
This is done in |
|
I managed to somewhat simplify the types. Going to merge this so that we can publish the blog post that includes a runnable example. We can further iterate in followup PRs. |
## [@helia/verified-fetch-v2.2.0](https://github.com/ipfs/helia-verified-fetch/compare/@helia/verified-fetch-2.1.3...@helia/verified-fetch-2.2.0) (2024-11-21) ### Features * support p2p retrieval by default ([#130](#130)) ([9d33f89](9d33f89))
|
🎉 This PR is included in version @helia/verified-fetch-v2.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Notes & open questions
Change checklist