Skip to content
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: create @helia/block-brokers package (#341) #342

Merged
merged 11 commits into from
Jan 8, 2024

Conversation

meandavejustice
Copy link
Contributor

  • moves packages/helia/src/block-brokers/ to packages/block-brokers
  • adds necessary boilerplate files and config for @helia/block-brokers package
  • updates all references to old block-brokers location in package/helia and top level config files

- moves `packages/helia/src/block-brokers/` to `packages/block-brokers`
- adds necessary boilerplate files and config for `@helia/block-brokers` package
- updates all references to old block-brokers location in `package/helia` and top level config files
Copy link
Member

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initial review, 4 files not fully looked at

.release-please-manifest.json Outdated Show resolved Hide resolved
packages/block-brokers/CHANGELOG.md Outdated Show resolved Hide resolved
packages/block-brokers/README.md Outdated Show resolved Hide resolved
packages/block-brokers/package.json Outdated Show resolved Hide resolved
packages/block-brokers/src/utils/networked-storage.ts Outdated Show resolved Hide resolved
* Networked storage wraps a regular blockstore - when getting blocks if the
* blocks are not present Bitswap will be used to fetch them from network peers.
*/
export class NetworkedStorage implements Blocks, Startable {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we still need to replace networked-storage in helia core right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its still there, I just duplicated this file. I figured we could discuss if it makes sense to pull into a @helia/utils package or something since it's used in multiple places

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think networked storage is just a wrapper for the blockStore and the blockBrokers (see https://github.com/ipfs/helia#-system-diagram), so we should be able to encapsulate networked-storage stuff within block-brokers for Helia fully, but we should get Alex's input.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes I see, moving it seems to make sense, if Alex wants something else we can change before merge

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved in e18e820

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love having NetworkedStorage and default-hashers in @helia/block-brokers since they are not block brokers, but let's merge this to get a next version out and figure out a refactor before final release.

packages/block-brokers/test/trustless-gateway.spec.ts Outdated Show resolved Hide resolved
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another file that I had duplicated @SgtPooki, it's also used in helia.ts

Davids-Computer helia % git grep -i "default-hashers"
packages/block-brokers/test/block-broker.spec.ts:import { defaultHashers } from '../src/utils/default-hashers.js'
packages/block-brokers/test/utils/networked-storage.spec.ts:import { defaultHashers } from '../../src/utils/default-hashers.js'
packages/block-brokers/typedoc.json:    "./src/utils/default-hashers.ts"
packages/helia/package.json:      "types": "./dist/src/utils/default-hashers.d.ts",
packages/helia/package.json:      "import": "./dist/src/utils/default-hashers.js"
packages/helia/src/helia.ts:import { defaultHashers } from './utils/default-hashers.js'
packages/helia/typedoc.json:    "./src/utils/default-hashers.ts"

Could move over and just export from block-brokers like NetworkedStorage

@meandavejustice meandavejustice marked this pull request as ready for review December 8, 2023 22:36
@meandavejustice meandavejustice requested a review from a team as a code owner December 8, 2023 22:36
files: block-brokers/ from helia package
@meandavejustice
Copy link
Contributor Author

All tests pass locally.

@meandavejustice
Copy link
Contributor Author

I'm still unclear on how release-please will handle the publishing of @helia/block-brokers. Also, will this be the proper place to update release-please-manifest.json with a new version for packages/helia and @helia/interop?

@SgtPooki SgtPooki mentioned this pull request Dec 14, 2023
3 tasks
@achingbrain achingbrain merged commit cdcf553 into ipfs:main Jan 8, 2024
18 checks passed
achingbrain added a commit that referenced this pull request Jan 8, 2024
- moves `packages/helia/src/block-brokers/` to `packages/block-brokers`
- adds necessary boilerplate files and config for `@helia/block-brokers` package
- updates all references to old block-brokers location in `package/helia` and top level config files

---------

Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com>
Co-authored-by: Alex Potsides <alex@achingbrain.net>
This was referenced Jan 8, 2024
@meandavejustice meandavejustice deleted the 341-breakout-block-brokers branch January 8, 2024 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants