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

Consolidate IPFS Path libraries under boxo/path #198

Closed
lidel opened this issue Mar 8, 2023 · 4 comments · Fixed by #334
Closed

Consolidate IPFS Path libraries under boxo/path #198

lidel opened this issue Mar 8, 2023 · 4 comments · Fixed by #334
Assignees
Labels
need/triage Needs initial labeling and prioritization

Comments

@lidel
Copy link
Member

lidel commented Mar 8, 2023

How many clowns path deps can you fit in a clown car one line of code?

During review of Gateway API refactor from #176, I've realized there are cases, when we use three "path types" from three different libraries. This culminated in this fun one-liner:

imPath, err := NewImmutablePath(ifacepath.New(ipath.String()))

Proposed path forward

Thoughts on deduplicating/consolidating under boxo/path?
It honestly feels like if go-libipfs has a purpose, it is to provide a solution for reusable things like this.

I feel we will make better Gateway API in #176 if we don't use ifacepath but use a single go-libipfs/path library that covers Path, Immutable and Resolved states with distinct types/interfaces.

I assume we want to follow "copy" strategy from #191 here, to minimize blast radius?

We could make this consolidation in steps, first switch gateway, so the public interface from #176 only uses types from boxo/path, and in the future switch namesys/kubo to that as well.

Things to look out for

During the consolidation, the following issues and Pull Requests from the relevant repositories should be taken into consideration.

cc @ipfs/kubo-maintainers for feedback (paths are everywhere, and as much as I'd like to clean this up, I don't want to add unnecessary work for anyone)

@lidel lidel added the need/triage Needs initial labeling and prioritization label Mar 8, 2023
@hacdias
Copy link
Member

hacdias commented Mar 8, 2023

I'm up for consolidating the three repositories/types into a single package in go-libipfs. I've also asked myself before why we need to have different path types and change between both in different places. I agree that we should follow a different strategy than for the remaining repositories as this is more of a merge and consolidation kind-of-thing so it's slightly different.

@guseggert
Copy link
Contributor

I agree this looks like it could be cleaned up. I would wait for the consolidation dust to settle before merging refactors like this though, just to make it a bit easier for consumers to upgrade.

@BigLep
Copy link
Contributor

BigLep commented Mar 8, 2023

Agreed. Lets get the "vX" release out that doesn't have any refactoring so there is a way for consumers to more easily update. Then cleanup/refactor.

@BigLep
Copy link
Contributor

BigLep commented Mar 10, 2023

Here is the tracking issue for the over the "over the hump" release: #202 . This refactor won't get merged into main until after that issue is completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/triage Needs initial labeling and prioritization
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants