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

Check if Error object has stack property. Fixes #34. #38

Merged
merged 2 commits into from
Jun 7, 2018
Merged

Check if Error object has stack property. Fixes #34. #38

merged 2 commits into from
Jun 7, 2018

Conversation

robertoschiavone
Copy link
Contributor

This PR addresses the problem described in #34

When trying to log an Error with an undefined stack trace, now signale correctly logs the error name and ignores the stack property.

Before:

const [name, ...rest] = msg.stack.split('\n');
                                        ^
TypeError: Cannot read property 'split' of undefined

After:

✖  error     SyntaxError

@passcod
Copy link

passcod commented Jun 6, 2018

If you .toString() the error instead, and replace the missing stack with that, it avoids losing so much information.

@klaudiosinani klaudiosinani added the enhancement New feature or request label Jun 7, 2018
@klaudiosinani klaudiosinani self-requested a review June 7, 2018 14:24
signale.js Outdated
@@ -192,7 +192,7 @@ class Signale {
}
}

if (msg instanceof Error) {
if (msg instanceof Error && typeof msg.stack !== 'undefined') {
Copy link
Owner

@klaudiosinani klaudiosinani Jun 7, 2018

Choose a reason for hiding this comment

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

Nice approach! We could also rely upon type conversion and prevent any falsy value from making it through, that way rare cases where stack is set, for example, to null are covered too;

if (msg instanceof Error && msg.stack) {

Copy link
Owner

@klaudiosinani klaudiosinani left a comment

Choose a reason for hiding this comment

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

A minor tweak and it is good to go! Thank you for taking the time to contribute : )

As per @klauscfhq request, the check now relies upon type conversion to prevent any
falsy value from making it through.
@robertoschiavone
Copy link
Contributor Author

@passcod can you provide an example of that? I tried to reproduce your .toString() suggestion but I obtained the exact same results.

Obviously this is the smallest tweak ever committed, so I guess that any further improvement would be welcome.

@klaudiosinani klaudiosinani merged commit b45d5b4 into klaudiosinani:master Jun 7, 2018
@klaudiosinani
Copy link
Owner

Thank you!

@robertoschiavone robertoschiavone deleted the bugfix/issue-34 branch June 7, 2018 21:07
@passcod
Copy link

passcod commented Jun 7, 2018

@robertoschiavone ah, nevermind, yes you're correct. Thanks for the PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants