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

Update websites.json: add a Nostr client #2182

Merged
merged 2 commits into from
May 2, 2023

Conversation

BlowaterNostr
Copy link
Contributor

Describe the changes you have made in this PR

Add https://blowater.deno.dev/ to Nostr website page

Type of change

  • docs: Documentation update

@BlowaterNostr BlowaterNostr changed the title Update websites.json Update websites.json: add a Nostr client Mar 8, 2023
@reneaaron
Copy link
Contributor

reneaaron commented Mar 8, 2023

@BlowaterNostr Thanks for the PR! I just wanted to test this application but stumbled upon this hint:

image

Do you mind to explain what isn't working for you in Firefox?

@BlowaterNostr
Copy link
Contributor Author

@reneaaron Will reproduce and give you a better report.

@reneaaron
Copy link
Contributor

@BlowaterNostr Cool, thanks! 🙌 Feel free to open a new issue for that 👍

@BlowaterNostr
Copy link
Contributor Author

Maybe it's not necessarily a bug of Alby. But the way my application is interacting with it. The direct observation is that Using Alby on Firefox with Blowater will trigger a null reference exception very deep in Preact's code with no call stack from application code. With the same code, but Chrome works.

@reneaaron
Copy link
Contributor

Okay. Is the code available publicly? Happy to take a quick look...

@github-actions
Copy link

github-actions bot commented Mar 8, 2023

🚀 Thanks for the pull request!

Here are the current build files for testing:

Download and unzip the file for your browser. Refer to the readme for detailed install instructions.


This build is brought to you by: channel.ninja (who recently dropped 1000 sats):


Want to sponsor the next build? send some sats to ⚡️builds@getalby.com (don't forget to provide your name)

Don't forget: keep earning sats!

@BlowaterNostr
Copy link
Contributor Author

@reneaaron the code is not open sourced yet. I will consider it once it reaches feature stability.

@BlowaterNostr
Copy link
Contributor Author

BlowaterNostr commented Mar 11, 2023

@reneaaron I have found the reason and it's a good indicator that Alby should support concurrent API calls or at least throw errors when user concurrently calls.

The story is this, in Blowater, we concurrently call encrypt/decrypt/signEvent/getPublicKey in many different promise chains. The current behavior of window.nostr is to simply return the result of the first call in the current JS tick. For example, if

getPublicKey()
encrypt()

are called in the same tick. await encrypt() will return the public key. Although in the log Alby gives a warning message, but the code behavior is not a good design. I would categorize it as either a bug or a design flaw.

I worked around this problem by adding a queue in my application code to wait for the previous call to resolve before invoking the next.

But, because Blowater also use IndexedDB, therefore, wrong data (should be an event, but was {enabled: true}) before I added the queue were persisted. I happened to never delete the database on Firefox, so that only Firefox appears buggy. I deleted the database and everything works now.

Fundamentally, TS is not type safe when it comes to serialize raw JSON to objects. Therefore, if application code always do type check during de serialization at runtime, this bug can be prevented. But this is not the norm in TS/JS code.

The communicate between extension and application is similar to serialization because it's also across program boundaries. We need better type safety here.

On Alby side, it should not return incorrect data (silent error) even if concurrent call is not allowed. Throwing exceptions is acceptable if concurrent call needs more time to design.

@reneaaron
Copy link
Contributor

@BlowaterNostr Thanks for this detailed report, we're looking into it! I'll get back to you as soon as we have a fix for it ready to test👌

@reneaaron reneaaron mentioned this pull request Mar 15, 2023
@BlowaterNostr
Copy link
Contributor Author

#2218 does not block the development of my client, can we merge this first? @reneaaron

@BlowaterNostr
Copy link
Contributor Author

Since #2218 is just merged, can we merge this now?

@reneaaron reneaaron merged commit 5111f3c into getAlby:master May 2, 2023
@BlowaterNostr
Copy link
Contributor Author

@reneaaron
image

Blowater isn’t showing up. Is this because the action failed?

@bumi
Copy link
Collaborator

bumi commented May 7, 2023

@BlowaterNostr you might stil be on the old version.
CleanShot 2023-05-07 at 11 42 38@2x

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.

3 participants