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

Enhanced Error Reporting for Improved Debugging and User Experience #100

Closed
wants to merge 2 commits into from

Conversation

Sadaf-A
Copy link
Contributor

@Sadaf-A Sadaf-A commented Feb 25, 2024

Resolves: #99

This PR adds a new class for throwing errors and gives more descriptive errors

This is what our errors look like now:
error(1)

This is what they will look like after the changes introduced by this PR are merged:
error-now

This PR also makes the sendMessage() method in websocket.ts async which is required to get the proper error object

@Sadaf-A
Copy link
Contributor Author

Sadaf-A commented Feb 25, 2024

just made a commit which sends the error code along with the message
now the error message looks like this:
code

Copy link

@Kanai2003 Kanai2003 left a comment

Choose a reason for hiding this comment

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

remove unnecessary spaces in websocket.ts

@Sadaf-A
Copy link
Contributor Author

Sadaf-A commented Feb 25, 2024

remove unnecessary spaces in websocket.ts

hey @Kanai2003, thanks for your review those spaces are for indentation. Any issue related to formatting will be resolved if my PR for integrating es-lint and prettier gets merged

@Kanai2003
Copy link

Oh! my bad!

Copy link
Member

@shalithasuranga shalithasuranga left a comment

Choose a reason for hiding this comment

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

Hello @Sadaf-A, Thanks for sending this pull request. I would like to skip adding these suggestions due to the following reasons:

  • It's up to app developers handle Neutralinojs errors - not the framework itself. i.e., clipboard.writeText() throws an error, so app developers need to handle it with try/catch
  • We realdy expose error code types for TypeScript

However, if more app developers mention that they need to see all uncaught errors on the console as formatted error messages, we can do something like this I think:

nativeCalls[message.id].reject(message.data.error);
if(onDebugMode) {
     logError(); // <----
}

Let's skip adding this feature for now 🎉

@Sadaf-A
Copy link
Contributor Author

Sadaf-A commented Feb 29, 2024

Hello @Sadaf-A, Thanks for sending this pull request. I would like to skip adding these suggestions due to the following reasons:

* It's up to app developers handle Neutralinojs errors - not the framework itself. i.e., `clipboard.writeText()` throws an error, so app developers need to handle it with `try/catch`

* We realdy expose error code types for TypeScript

However, if more app developers mention that they need to see all uncaught errors on the console as formatted error messages, we can do something like this I think:

nativeCalls[message.id].reject(message.data.error);
if(onDebugMode) {
     logError(); // <----
}

Let's skip adding this feature for now 🎉

Hey, thanks for the review. From that perspective it makes more sense to keep things the way they are!

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.

Enhanced Error Reporting for Improved Debugging and User Experience
3 participants