-
Notifications
You must be signed in to change notification settings - Fork 106
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: BlockBroker factory support #284
feat: BlockBroker factory support #284
Conversation
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.
self review
@@ -19,6 +20,7 @@ const log = logger('helia') | |||
interface HeliaImplInit<T extends Libp2p = Libp2p> extends HeliaInit<T> { | |||
libp2p: T | |||
blockstore: Blockstore | |||
blockBrokers: BlockBroker[] |
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.
needed to ensure it's not using the Array<BlockBroker | BlockBrokerFactoryFunction>
type from HeliaInit
return blockBroker({ | ||
blockstore, | ||
datastore, | ||
libp2p, | ||
hashers | ||
}) satisfies BlockBroker |
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.
are there other things that blockBrokers may need?
@@ -98,7 +98,7 @@ export interface HeliaInit<T extends Libp2p = Libp2p> { | |||
* A list of strategies used to fetch blocks when they are not present in | |||
* the local blockstore | |||
*/ | |||
blockBrokers?: BlockBroker[] | |||
blockBrokers?: Array<BlockBroker | BlockBrokerFactoryFunction> |
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.
do we just want to accept BlockBrokerFactoryFunction[]
instead of BlockBrokers
directly?
return createNode({ | ||
blockBrokers: [ | ||
BitswapBlockBrokerFactory | ||
], |
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 should not be making external network requests (i.e. not calling default trustlessGateways) during tests.
/** | ||
* A function that receives some {@link Helia} components and returns a | ||
* {@link BlockBroker}. | ||
* | ||
* This is needed in order to re-use some of the internal components Helia | ||
* constructs without having to hoist each required component into the top-level | ||
* scope. | ||
*/ | ||
export interface BlockBrokerFactoryFunction { | ||
(heliaComponents: BlockBrokerFactoryComponents): BlockBroker | ||
} |
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.
seemed like the best place for this but lmk if somewhere else is better.
@@ -15,7 +15,7 @@ import type { BitswapBlockBroker } from '../../src/block-brokers/bitswap-block-b | |||
import type { Blockstore } from 'interface-blockstore' | |||
import type { CID } from 'multiformats/cid' | |||
|
|||
describe('storage', () => { | |||
describe('networked-storage', () => { |
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.
this has the same describe namespace as packages/helia/test/storage.spec.ts
and it bugs me
@@ -25,6 +25,7 @@ describe('pins (recursive)', () => { | |||
dag = await createDag(codec, blockstore, 2, 3) | |||
|
|||
helia = await createHelia({ | |||
blockBrokers: [], |
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.
Prevent blockBroker networking when not needed
@@ -27,6 +27,7 @@ describe('pins (depth limited)', () => { | |||
dag = await createDag(codec, blockstore, MAX_DEPTH, 3) | |||
|
|||
helia = await createHelia({ | |||
blockBrokers: [], |
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.
Prevent blockBroker networking when not needed
This approach is consistent with libp2p components, unixfs, ipns, etc. Since #281 and #284 were merged without review, this PR implements suggsestions that would have been in the review of those PRs. 1. Creation of block brokers is done by exported function. If your broker takes arguments, pass them to the factory function. The factory then returns a function that accepts helia components and returns the broker. 2. Removes BitswapBrokerFactory as it is redunant The internal API may need some more work but the external API should be relatively stable.
Adds on to changes from #280 and #281
This makes it much easier to support constructing helia with custom blockBrokers while still allowing consumers easy access to the default BitswapBlockBroker. One particular use-case where this comes in handy: our tests.
Take the code in
packages/helia/test/fixtures/create-helia.ts
for example. Without these changes, you have to do something like:However, with these changes, we can now do this:
Some key benefits:
BitswapBlockBroker
createLibp2p