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

Replace nostr-hooks with NDK #21

Merged
merged 16 commits into from
Jan 20, 2024
Merged

Conversation

sepehr-safari
Copy link
Contributor

  • formatted with prettier
  • refactored existing code base
  • replace nostr-hooks with ndk
  • add ndk-cache-dexie to use caching mechanism

this pr should solve this #14

considerations

please review and test all functionalities before merging.
there was no test cases and I didn't know much about the context so was unable to test it manually.

@rolznz
Copy link
Collaborator

rolznz commented Jan 15, 2024

@sepehr-safari thanks for the PR! I was originally thinking caching could be added to nostr-hooks to support the feature. Why did you decide to use NDK instead (and then this PoS no longer uses your library)? is it unfeasable to add caching to nostr-hooks right now?

I will try review and get this merged before the end of the week.

Regarding the context: it's a simple Point of Sale with the ability to manage your items on nostr. This is possible by using the private key of the NWC URL to sign events. All you need is to connect a wallet (or you can try mine which already has a public profile and some items attached to it)

@sepehr-safari
Copy link
Contributor Author

caching could be added to nostr-hooks. is it unfeasable to add caching to nostr-hooks right now?

that would be possible. but for now, unfortunately, I don't have a plan.

Why did you decide to use NDK instead?

NDK is a masterpiece by Pablo. it's feature-rich, super easy-to-use, and up-to-date with the edge of Nostr. I suggest using it over my nostr-hooks these days.
btw, I'm planning to start refactoring nostr-hooks powering by NDK. that would bring best of both worlds.

this PoS no longer uses your library?

yeap. totally replaced with NDK and NDK-cache-dexie.

I will try review and get this merged before the end of the week.

good luck. ping me in case of any issue.

Copy link
Collaborator

@rolznz rolznz left a comment

Choose a reason for hiding this comment

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

tACK

}

interface Actions {
setProvider(provider: webln.NostrWebLNProvider | undefined): void;
addItemToCart(item: Item): void;
removeItemFromCart(item: Item): void;
clearCart(): void;
addRelay(relay: string): void;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these 2 methods were supposed to address this TODO:

// TODO: allow dynamic list of relays

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sepehr-safari I removed them since they are not actually used (and I think we need to consider the UX there)

@rolznz
Copy link
Collaborator

rolznz commented Jan 20, 2024

@sepehr-safari I just tried it out now. There were several issues but overall quite impressive considering you didn't test it!

showing items and the user profile is super fast now! 🚀

There are a few strange things about ndk - I couldn't see how to disconnect a subscription or disconnect the instance itself which would probably be good to figure out, but the app works as-is right now.

And there is some additional management needed now since there are no nice hooks. Do let me know if you update nostr-hooks to be powered by ndk, I will use it in my next app!

@rolznz rolznz merged commit cba39b0 into getAlby:master Jan 20, 2024
@sepehr-safari
Copy link
Contributor Author

great to hear!

I couldn't see how to disconnect a subscription

you can use the option closeOnEose: true | false on each subscription, or on the ndk instance. it's about your flow. if you want to stay connected and receive new events in real-time you can use closeOnEose: false (current setup)

And there is some additional management needed now since there are no nice hooks. Do let me know if you update nostr-hooks to be powered by ndk, I will use it in my next app!

thank you for this motivation. I will do.

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.

2 participants