search

JS-DevTools / ono

Wrap errors without losing the original message, stack trace, or properties
https://jstools.dev/ono/
MIT License
106 stars 11 forks source link

Combine exception stacks lazily where possible #1

Closed glenjamin closed 8 years ago

glenjamin commented 8 years ago

This is a bit wacky, but I figured I'd send in the PR and see what you think.

I did a big survey of error / exception libs I could find on npm today, and this one ticked the boxes I was looking for - attaching arbitrary data, and concatenating stack traces together.

One thing I was hoping to find was a lib which only attached the stack traces together if the .stack property is read - especially on v8 where this property is lazily calculated.

This PR attempts to detect if this is possible in the executing runtime, and then does so.

Passes tests on Karma and Node locally, but there might be something I've missed still

JamesMessinger commented 8 years ago

Omigod that rocks! Great idea. I'll do some testing of my own and maybe even add some new tests to specifically cover this, but yes, I'll be glad to merge this PR.

JamesMessinger commented 8 years ago

Actually... I may not have the time to get to this myself for a week or two. So why wait? :)

Would you mind adding a test case to hit this line of code? It's the only branch that's not currently covered. If you'll do that, I'll merge the PR ASAP

glenjamin commented 8 years ago

Is the coverage only from node?

I can patch a global to simulate the fallback to hit that case.

JamesMessinger commented 8 years ago

The coverage data includes node and browsers. (all the lcov.info files are concatenated together)

However, I just noticed that I only have KARMA_COVERAGE=true set for one of the CI builds, and that buid only includes Firefox and PhantomJS. Try setting KARMA_COVERAGE=true for the SauceLabs build. That build covers all browsers, so maybe one of them will hit that line of code.

glenjamin commented 8 years ago

I'm fairly sure firefox should hit that line, i'll give it a go.

glenjamin commented 8 years ago

That's all passing now and executing the line from before (older versions of firefox made .stack a property descriptor that wasn't lazy)

As part of these changes I broke a long line up and because this is line coverage and not branch coverage, it's not getting hit any more.

The line in question is https://coveralls.io/builds/4946993/source?filename=lib%2Findex.js#L192 which can only be hit if Object.getOwnPropertyDescriptor && Object.defineProperty is false.

This is only needed for IE8 support - is that a goal?

JamesMessinger commented 8 years ago

IE8 support isn't necessary, but it doesn't hurt to leave that line in there just in case. I'm ok with it not being hit.

Thanks for all your work! Merging in 3... 2... 1...

JamesMessinger commented 8 years ago

Onoes! The tests are failing on Android due to the _error property that gets added by the extendStackProperty() method.

I'll make a quick change to avoid adding that property.

glenjamin commented 8 years ago

Thanks! Do the android tests only run on the main branch?

JamesMessinger commented 8 years ago

I have no idea why the Android tests didn't fail on the PR branch. They appear to have run successfully there. No clue.