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

Fix relay sub/unsub before connection is established #73

Closed
wants to merge 5 commits into from

Conversation

adamritter
Copy link
Contributor

No description provided.

…ses independent, let sub/unsub happen before connection was estabilished
@adamritter
Copy link
Contributor Author

This implementation also doesn't send REQ if the subscription was cancelled before connection was established

@adamritter
Copy link
Contributor Author

I added a bug fix for unsubscribing (the code was still trying to send a CLOSE request in connecting state)

@@ -12,7 +12,7 @@ export type Relay = {
close: () => Promise<void>
sub: (filters: Filter[], opts?: SubscriptionOptions) => Sub
publish: (event: Event) => Pub
on: (type: RelayEvent, cb: any) => void
on: (type: RelayEvent, cb: (event: Event)=>void) => void
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not correct because it doesn't apply when the event received is a connect, disconnect or notice.

@fiatjaf
Copy link
Collaborator

fiatjaf commented Jan 2, 2023

Instead of just guarding these things under ifs we should wait until the connection is established and then send the messages.

@adamritter
Copy link
Contributor Author

Instead of just guarding these things under ifs we should wait until the connection is established and then send the messages.

Sure, we can do that, though I'm planning to do a RelayPool and smarter Relay (with auto reconnection, looking at timeouts) where the reconnecting and error handling is done by the Relay itself. I though it would be better to put it into nostr-tools as I see that there are many RelayPool implementations already. I wanted to ask you if there's a better way to reach you (4 days is a bit too long for answer on your plans for nostr-tools). Is there another official way? (I wrote DM on nostr as well).

I wrote a caching storage layer using IndexDB, that is independent of frameworks (only depends on nostr-tools and dexie.js, but I'm not against dropping dexie.js / adding other db backends). It's closed source because it's not ready yet, but it will be open source.

I also wrote a Svelte client on top of the caching storage layer, but I make sure that it can be used by other clients as well that have access to JavaScript.

@fiatjaf
Copy link
Collaborator

fiatjaf commented Jan 2, 2023

You can talk to me on the https://t.me/nostr_protocol channel (I am not using Nostr DMs these days) but please don't demand attention or quick responses.

I feel like you didn't understand my request above. This is not related to relay pool. There is even no relay pool implementations here anymore (I've removed the one that existed).

Publish your libraries and clients and talk about them on Telegram and Nostr and Twitter and see if people use them.

@adamritter
Copy link
Contributor Author

Sounds great, I added that Telegram channel (maybe it should be on the home page as an alternative contact).

I understood the request, I just wrote about the RelayPool because I believe that handling connect / disconnect / reconnection logic should be done by the Relay class when we're building abstractions over it. The current Relay implementation is a leaky abstraction that is a great code example, but using it didn't make my code shorter when I added connection handling logic to it that was independent of subscription logic.

It's OK to have different opinions about it, I accept yours, thanks for your answer.

@adamritter
Copy link
Contributor Author

I created and published a separate relay pool library on top of nostr-tools, as you don't want it to be part of nostr-tools.

https://github.com/adamritter/nostr-relaypool-ts

It would be great to link to it from this repo, as they work well together (having a link to every project that uses nostr-tools would be great actually)

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

Successfully merging this pull request may close these issues.

None yet

2 participants