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: export dag walkers #414

Closed
wants to merge 3 commits into from
Closed

Conversation

saul-jb
Copy link
Contributor

@saul-jb saul-jb commented Feb 4, 2024

This simple PR just exports the dag-walkers functionality out of utils saving the need to re-implement it if it is required.

@saul-jb saul-jb requested a review from a team as a code owner February 4, 2024 23:50
@achingbrain
Copy link
Member

Thanks for opening this PR.

Could you speak a little to the use-case that requires exporting these separately?

The default set of DAG Walkers are always configured on a Helia node and are available via the .dagWalkers property - any extras passed to the config are merged in to the default list.

@saul-jb
Copy link
Contributor Author

saul-jb commented Feb 6, 2024

Could you speak a little to the use-case that requires exporting these separately?

It's simply using them outside of Helia. This file implements the most common DAG walkers and rather than copy/paste them into my project it makes sense to just be able to import them.

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.

I think exporting dag-walkers (and other code here) would be useful for consumers, but the real question is, do we want to maintain support for these here.

@SgtPooki
Copy link
Member

We discussed this in the Helia WG last week. I think we would need to move these to a separate utils package that can be maintained by the community instead of surfacing as a part of the public Helia API

@achingbrain
Copy link
Member

achingbrain commented Feb 22, 2024

The suggestion on maintainers call was to add a method to each IPLD codec that yields CIDs from a block corresponding to that codec, but I don't even think we need to do that - we can use the existing links() function on a multiformats/block to yield them instead.

This is how it used to be done in js-ipfs: https://github.com/ipfs/js-ipfs-repo/blob/master/packages/ipfs-repo/src/utils/walk-dag.js

We may end up removing the dag walkers here entirely to reduce code bloat, which is also why things shouldn't be exported unless they are being used.

@achingbrain
Copy link
Member

I'm going to close this as it's not necessary to export these as part of the Helia API, though thank you for opening this PR and for taking the time to explain your reasoning.

Follow-up: #447

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants