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: refactor actions and add tests #2015

Merged
merged 3 commits into from
Mar 2, 2023
Merged

chore: refactor actions and add tests #2015

merged 3 commits into from
Mar 2, 2023

Conversation

im-adithya
Copy link
Member

Describe the changes you have made in this PR

Adds tests to actions in the background-script
Migrates actions to TypeScript

Type of change

  • chore: Refactoring (non-breaking change to the code)

How has this been tested?

Wrote unit tests

Checklist

  • My code follows the style guidelines of this project and performed a self-review of my own code
  • New and existing tests pass locally with my changes
  • I checked if I need to make corresponding changes to the documentation (and made those changes if needed)

@github-actions
Copy link

github-actions bot commented Jan 27, 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!

@im-adithya im-adithya marked this pull request as ready for review February 20, 2023 10:37
@im-adithya im-adithya requested review from reneaaron and escapedcat and removed request for reneaaron February 20, 2023 10:37
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.

utack

@bumi bumi requested a review from rolznz February 23, 2023 23:24
@@ -20,7 +21,9 @@ const makeInvoice = async (message, sender) => {
});
return response;
} catch (e) {
return { error: e.message };
if (e instanceof Error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if e is not an Error? will this exception be swallowed?

I think it would be great to have a helper function we can use to get a message from an exception

Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't you mention this recently somewhere else? Wanna create an issue for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's because of TypeScript; it should always be an error (according to the makeInvoice function)

But yeah, we use this repeatedly and it's best to have a helper function which somehow extracts the message from exception var. Not sure how we do that, though.

@bumi bumi merged commit 19a6167 into master Mar 2, 2023
@bumi bumi deleted the task-refactor-actions branch March 2, 2023 10:19
This pull request was closed.
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.

4 participants