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

Define message for non-Error errors #69

Merged
merged 4 commits into from
Nov 8, 2024
Merged

Conversation

aryanjassal
Copy link
Contributor

@aryanjassal aryanjassal commented Nov 6, 2024

Description

If a non-Error error is thrown from within a RPC handler, the message field would be undefined. This would result in the message field (which is expected to be present) be omitted, breaking the parsing step.

This PR addresses that by wrapping that in a nicer message. For example, if we do throw 123, the message becomes 'Non-error class Number was thrown'. If we do throw new ReadableStream(), then the message becomes 'Non-error class ReadableStream was thrown'. You get the idea.

Issues Fixed

Tasks

  • 1. If the message does not exist, substitute it with something else
  • 2. Run tests to ensure this behaviour is consistent

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@aryanjassal aryanjassal self-assigned this Nov 6, 2024
@aryanjassal aryanjassal changed the title feat: properly defined message for non-Error errors Define message for non-Error errors Nov 6, 2024
@aryanjassal
Copy link
Contributor Author

I was able to make the message guessing function more complex, so now it should return something like this:

// For a number 123
> console.log(composeMessage(e));
Non-error literal 123 was thrown

// For regular messages
> console.log(composeMessage(e));
this was the error message

// For objects, like ReadableStream or Array
> console.log(composeMessage(e));
Non-error object ReadableStream was thrown

I'm not too happy about needing to make a separate function to handle this logic as it won't fit in one line, and is required to be repeated, so this can't be defined where needed. Rather, I had to define composeMessage inside the RPCServer.ts itself. I didn't export it, so it becomes like a private utility function.

Thoughts, @tegefaulkes?

src/RPCServer.ts Outdated Show resolved Hide resolved
src/RPCServer.ts Outdated Show resolved Hide resolved
src/RPCServer.ts Outdated Show resolved Hide resolved
@aryanjassal
Copy link
Contributor Author

All the tasks have been completed, and this PR has been approved. Merging.

@aryanjassal aryanjassal merged commit 3216c85 into staging Nov 8, 2024
8 checks passed
@CMCDragonkai
Copy link
Member

Generally I prefer not to make application concerns a library functionality. The exact reporting of a message - specifically it's template or toString impl should be generic and allowed to be overridden by the downstream user. This is how you ensure reusable modules/libraries.

@aryanjassal
Copy link
Contributor Author

Generally I prefer not to make application concerns a library functionality. The exact reporting of a message - specifically it's template or toString impl should be generic and allowed to be overridden by the downstream user. This is how you ensure reusable modules/libraries.

Should I make an issue to track this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants