Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

feat: Add pin.add to ipfs-message-port #3575

Closed
wants to merge 37 commits into from
Closed

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented Feb 27, 2021

Builds upon work @icidasset has started in #3497, resolving remaining issues and added refactoring to reuse interfaces.

@BigLep BigLep added the status/in-progress In progress label Mar 15, 2021
@icidasset
Copy link
Contributor

👋 Any status update on this? Let me know if I can help.

@Gozala
Copy link
Contributor Author

Gozala commented Apr 12, 2021

👋 Any status update on this? Let me know if I can help.

@icidasset I'd love to finish this, but had been moved to a team with diff focus making it challenging. That said, I'll be able to spend time on this next week where I'll try to get this to a reviewable state.

If you do have time and want to pick it up that would be a great help.

@icidasset
Copy link
Contributor

@icidasset I'd love to finish this, but had been moved to a team with diff focus making it challenging. That said, I'll be able to spend time on this next week where I'll try to get this to a reviewable state.

If you do have time and want to pick it up that would be a great help.

I'm quite busy too, but I'd say, leave a todo list here, and I'll try to help out whenever I can ☺️
Would love to get this in too, since we've been running a fork for some time now.

I guess this goes for #3573 too

@Gozala
Copy link
Contributor Author

Gozala commented Apr 12, 2021

leave a todo list here, and I'll try to help out whenever I can ☺️

There is no really a list, it was all working locally on my device but seemed to fail on CI figuring out why and getting it green is basically what's remaining (I'm pretty sure it was unrelated to changes here, just was bad timing where tree got red because of some dep version bump).

That said I see lot of conflicts now so merging changes from upstream and getting those conflicts resolved would be added piece of work.

@@ -122,6 +122,7 @@
"uint8arrays": "^2.1.3"
},
"devDependencies": {
"libp2p-interfaces": "^0.8.4",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise

src/ipns/routing/config.js:26:36 - error TS2345: Argument of type 'import("/Users/gozala/Projects/js-ipfs-2/node_modules/libp2p/node_modules/libp2p-interfaces/dist/src/pubsub/index")' is not assignable to parameter of type 'import("/Users/gozala/Projects/js-ipfs-2/node_modules/libp2p-interfaces/dist/src/pubsub/index")'.
  Types of property 'peers' are incompatible.
    Type 'Map<string, import("/Users/gozala/Projects/js-ipfs-2/node_modules/libp2p/node_modules/libp2p-interfaces/dist/src/pubsub/peer-streams")>' is not assignable to type 'Map<string, import("/Users/gozala/Projects/js-ipfs-2/node_modules/libp2p-interfaces/dist/src/pubsub/peer-streams")>'.
      Type 'import("/Users/gozala/Projects/js-ipfs-2/node_modules/libp2p/node_modules/libp2p-interfaces/dist/src/pubsub/peer-streams")' is not assignable to type 'import("/Users/gozala/Projects/js-ipfs-2/node_modules/libp2p-interfaces/dist/src/pubsub/peer-streams")'.
        Types have separate declarations of a private property '_rawOutboundStream'.
26     pubsubDs = new PubsubDatastore(pubsub, localDatastore, peerId)

@BigLep
Copy link
Contributor

BigLep commented May 1, 2021

@Gozala : #3607 has merged. Is this unblocked now?

@lidel
Copy link
Member

lidel commented May 10, 2021

Seems stale? Feel free reopen if still relevant.

@lidel lidel closed this May 10, 2021
@Gozala Gozala reopened this May 11, 2021
@Gozala
Copy link
Contributor Author

Gozala commented May 11, 2021

It is still relevant and I want to land it, just was unable to resolve all the issues during my rotation and now there are bunch of new conflicts to resolve.

@expede
Copy link

expede commented Jun 10, 2021

Fission would be happy to take this over, unless there's any objections. We've been maintaining a fork, since we depend on this functionality. It would be nice to have this land in an official release.

It is still relevant and I want to land it, just was unable to resolve all the issues during my rotation and now there are bunch of new conflicts to resolve.

@Gozala is there any additional context that we need, or is it mostly about resolving the merge conflicts?

cc @icidasset @matheus23

@Gozala
Copy link
Contributor Author

Gozala commented Jun 22, 2021

I have updated pr and resolved all the issues, it's ready to be reviewed

@lidel lidel requested a review from achingbrain June 22, 2021 21:20
@lidel lidel marked this pull request as draft July 30, 2021 14:05
@expede
Copy link

expede commented Aug 31, 2021

@Gozala

Just a friendly follow up! Any movement / anything we can help with?

@lidel
Copy link
Member

lidel commented Sep 10, 2021

@expede I don't believe Irakli has bandwidth for this, if you are interested in picking this up and pushing towards the finish line, that would be fantastic 🙏

@lidel lidel added the help wanted Seeking public contribution on this issue label Oct 1, 2021
@lidel
Copy link
Member

lidel commented Oct 1, 2021

@expede @icidasset is this something you'd like to see ship sooner than later?
If so, feel free to open a new PR based on this branch and rebase it + CI is green and we will land it.

@icidasset
Copy link
Contributor

@lidel Not so urgent since we're not going to do any pinning anymore. Seeing that js-ipfs doesn't do garbage collection anyways.

@lidel
Copy link
Member

lidel commented Oct 8, 2021

Closing it for now, if anyone needs add please pick it up and open as a new one

@lidel lidel closed this Oct 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted Seeking public contribution on this issue status/in-progress In progress
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants