Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

PUBSUB: unsubscribe without handler reference #940

Closed
wants to merge 3 commits into from

Conversation

Isan-Rivkin
Copy link

@Isan-Rivkin Isan-Rivkin commented Feb 6, 2019

Hi :-)
following up on an issue in interface-js-ipfs-core I'm creating this PR.
Also created PR in js-libp2p accordingly.
This would help in cases like mine where my implementation is not able to support references to a handler function.
I will add the final url in the README to the method documentation once I merge the final PR to interface-js-ipfs-core since its pointing nowhere now.

Edit:

Opened a PR on interface-js-ipfs-core

@Isan-Rivkin
Copy link
Author

What can i do to increase codedov in this case? :-o

@jacobheun
Copy link
Contributor

This PR depends on ipfs-inactive/interface-js-ipfs-core#437. Once that is released it will resolve the code coverage issue here as we'll be running the updated pubsub test suite https://github.com/ipfs/interface-js-ipfs-core/pull/437/files#diff-c75d438d36bbcf7a883a20ff916c89d5

@alanshaw
Copy link
Contributor

@Isan-Rivkin can you update the interface-ipfs-core version so we can double check the tests pass in CI?

Note: we also need a PR to JS IPFS to implement this behaviour.

Thank you 🚀

@alanshaw
Copy link
Contributor

I think the error here is due to the fixtures location changing from "js/test/fixtures" to "test/fixtures" in the interface-ipfs-core project. I think if you do a find/replace for this it'll solve the issue.

@Isan-Rivkin
Copy link
Author

@alanshaw That's weird those tests works on my machine lol, will look at this, thanks for the hint!

@Isan-Rivkin
Copy link
Author

@alanshaw i tried to debug this but locally everything works, any clue would help 🙏 ?

@alanshaw
Copy link
Contributor

alanshaw commented Mar 9, 2019

I think that failure is unrelated - I'll be looking into that hopefully this week.

@alanshaw
Copy link
Contributor

@Isan-Rivkin I'm working on getting your work mergable here #956

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

Successfully merging this pull request may close these issues.

3 participants