-
Notifications
You must be signed in to change notification settings - Fork 123
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
fix: only traverse relevant part of HAMT #448
Conversation
if (result.unixfs?.type === 'hamt-sharded-directory') { | ||
// special case - unixfs v1 hamt shards | ||
dirCid = await findShardCid(result.node, part, blockstore) | ||
} else { | ||
dirCid = findLinkCid(result.node, part) |
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 code is straight-up copy-pasted from https://github.com/ipfs/js-ipfs-unixfs/blob/4749d9a7c1eddd86b8fc42c3fa47f88c7b1b75ae/packages/ipfs-unixfs-exporter/src/resolvers/unixfs-v1/index.ts#L59 which makes me wonder why we can't just use walkPath
from ipfs-unixfs-exporter directly and pass a path
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 using ipfs-unixfs-exporter
instead of copy-pasting the code is a better idea.
@@ -1,12 +1,14 @@ | |||
import { logger } from '@libp2p/logger' | |||
import { exporter } from 'ipfs-unixfs-exporter' | |||
import findShardCid from 'ipfs-unixfs-exporter/dist/src/utils/find-cid-in-shard.js' |
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 isn't right. We could copy-paste or export the code if needed, but wonder if we could use more of the resolver code from ipfs-unixfs-exporter as-is
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 you can't do deep imports of files like this any more, it's prevented by the exports
map in the module's package.json
which defines what's importable from a module and what isn't.
I have opened a PR that uses the It should also solve the issue reported in ipfs/service-worker-gateway#18 |
Closing in favor of #455 |
Description
Switched the UnixFS resolver code to do resolve only the relevant part of a HAMT rather than potentially enumerating all of it.
Fixes: ipfs/service-worker-gateway#18
Notes & open questions
Change checklist