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

chore: add types #60

Merged
merged 7 commits into from
Mar 16, 2021
Merged

chore: add types #60

merged 7 commits into from
Mar 16, 2021

Conversation

achingbrain
Copy link
Member

Needs a bit of sanity checking as this datastore appears to require
Uint8Arrays as keys when every other datastore takes Keys as keys.

Does not type the tests because they use lots of libp2p primitives
and the types there need a bit of work still.

Needs a bit of sanity checking as this datastore appears to require
`Uint8Arrays` as keys when every other datastore takes `Keys` as keys.

Does not type the tests because they use lots of libp2p primitives
and the types there need a bit of work still.
Copy link
Member

@hugomrdias hugomrdias left a comment

Choose a reason for hiding this comment

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

LGTM, this datastore really shouldn’t be called datastore but as far as types goes looks good

@achingbrain achingbrain marked this pull request as draft March 7, 2021 13:56
@achingbrain
Copy link
Member Author

Marked this as a draft as I'm going to integrate it with js-ipfs as a branch then circle back here and make sure the types reflect actual usage.

@vasco-santos
Copy link
Member

Uint8Arrays as keys when every other datastore takes Keys as keys.

This should be consistent with the DHT API. Perhaps the docs are the problem here, however this also is part of a TieredDatastore

@achingbrain
Copy link
Member Author

In IPFS we pass in the libp2p.pubsub property as the pubsub argument to this module's constructor, which has separate pubsub.on and pubsub.subscribe methods.

This module was passing the message handler as the second argument to pubsub.subscribe and not calling pubsub.on which means messages will never be received as subscribing and handling incoming messages are separate operations in libp2p.pubsub.

I've updated the code and tests to use the libp2p.pubsub interface as IPFS does at runtime.

@vasco-santos
Copy link
Member

@achingbrain could we do the same thing in js-ipfs regarding the subscribe/on? I would like to get rid of https://github.com/libp2p/js-libp2p/blob/master/src/pubsub-adapter.js

@achingbrain
Copy link
Member Author

could we do the same thing in js-ipfs regarding the subscribe/on?

Yes, I think so - it would be a case of doing something similar to that adapter in IPFS so we can keep the same external API.

@achingbrain achingbrain marked this pull request as ready for review March 15, 2021 07:55
Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

LGTM

FYI, I will be opening an issue to figure out the inconsistencies on datastore interface/dht for this repo , so that we track that

src/index.js Outdated Show resolved Hide resolved
@vasco-santos vasco-santos merged commit 7ea263a into master Mar 16, 2021
@vasco-santos vasco-santos deleted the chore/add-types branch March 16, 2021 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants