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: unify connector functions #1713

Merged
merged 2 commits into from
Nov 11, 2022
Merged

Conversation

bumi
Copy link
Collaborator

@bumi bumi commented Nov 5, 2022

We throw an error on the error case

Link this PR to an issue [optional]

fixes #1712

Type of change

(Remove other not matching type)

  • fix: Bug fix (non-breaking change which fixes an issue)

How has this been tested?

needs testing

We throw an error on the error case
@bumi bumi requested review from escapedcat and im-adithya November 5, 2022 22:36
@github-actions
Copy link

github-actions bot commented Nov 5, 2022

🚀 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: Adam Fiscor (who recently dropped 1337 sats):

The future is bright. We just have a lot of work to do.

Want to sponsor the next build? send some sats to ⚡️[email protected] (don't forget to provide your name)

Don't forget: keep earning sats!

Copy link
Contributor

@escapedcat escapedcat left a comment

Choose a reason for hiding this comment

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

Thanks! This makes a lot of sence

@escapedcat
Copy link
Contributor

Uhm, at one point in time the returned Error was used to trigger and show an error-toast. But this doesn't seem to work anymore (on master) so this change makes the current state better.
Not sure if we should to let the user know that their connector doesn't support this via a toast.

@bumi
Copy link
Collaborator Author

bumi commented Nov 6, 2022

I don't fully get your last comment. what's the suggestion?

@escapedcat
Copy link
Contributor

No suggestion. Your PR improves the current situation. This is good.

Question:
At one point in time the returned Error was used to trigger a toast within the extension to give the user feedback.
Currently the user doesn't get any feedback in the UI, right? Do we want to give feedback?

@bumi
Copy link
Collaborator Author

bumi commented Nov 7, 2022

why does the user not get feedback?

https://github.com/getAlby/lightning-browser-extension/pull/1713/files#diff-d07eb0245fd06ccaf878d056614cc4a944a918bbdd4267619cf3eb264bdd7eb3 -> here for example: both return error: message in the error case.

@escapedcat
Copy link
Contributor

image
Might a bug then? I don't see anything inside the UI currently.

@bumi
Copy link
Collaborator Author

bumi commented Nov 7, 2022

hmm. your error is strange. because getInvoices should be implemented on Lndhub

@escapedcat
Copy link
Contributor

hmm. your error is strange. because getInvoices should be implemented on Lndhub

All good, that was just a test by replacing the function

@bumi
Copy link
Collaborator Author

bumi commented Nov 7, 2022

@escapedcat
Copy link
Contributor

When #1080 was merged it looked like this:

But now that we support most of the important connectors we're good.

@bumi
Copy link
Collaborator Author

bumi commented Nov 8, 2022

naja, can you explain that? that PR is from Jul 26. so I don't know what you try to say.
is this related to this PR?

But now that we support most of the important connectors we're good.

we still don't support it in all. and if we broke something we should not say: 🤷 we do most ...

@bumi bumi requested a review from escapedcat November 8, 2022 23:06
@escapedcat
Copy link
Contributor

:D So, this is related to this PR because by looking at this I noticed that apparently "showing the toast to the user" does not work anymore. We need to decide we want to show something to the user if their connector doesn't support this yet.

  • If yes, close this PR and we open an issue to fix the toast
  • If no, merge this PR and keep it the current way

It's a decision of priorization

@bumi
Copy link
Collaborator Author

bumi commented Nov 9, 2022

this seems broken in master, no idea when/how it got broken (that could use investigation, because I have no idea how it could have worked before)

this PR actually fixes this I think for the invoices. at least it should. https://github.com/getAlby/lightning-browser-extension/pull/1713/files#diff-167ef8f8c43cebe91d5e71f3724e953a69277ab2c9b30af8dfc47a0707613544

what do we need to prioritize?

@bumi bumi merged commit c0ca5f9 into master Nov 11, 2022
@bumi bumi deleted the chore/unify-connector-functions branch November 11, 2022 10:09
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.

Unify connector interface
2 participants