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

fix(browser): Don't special case DOMExceptions that are also Errors #4156

Closed
wants to merge 1 commit into from
Closed

fix(browser): Don't special case DOMExceptions that are also Errors #4156

wants to merge 1 commit into from

Conversation

nowylie
Copy link

@nowylie nowylie commented Nov 15, 2021

As mentioned in #4085, DOMExceptions are not a legacy API and on current browsers are instances of Error. This change tests whether the exception is an instance of Error before throwing away all the good bits.

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

Fixes #4085
Fixes #3119

@AbhiPrasad AbhiPrasad self-requested a review November 15, 2021 14:00
@AbhiPrasad
Copy link
Member

Awesome, thanks for your contribution!

Can we please leave a comment above the new conditional explaining the instance check (maybe with some links to docs)? We can also update the comments below it that refer to DOMException as a legacy API.

@lobsterkatie
Copy link
Member

Echoing @AbhiPrasad, thanks for the contribution!

I modified your change slightly, so that we don't miss capturing the code property, and therefore am going to close this in favor of that version.

lobsterkatie added a commit that referenced this pull request Nov 15, 2021
Inspired by #4156 - h/t @nowylie.

According to the spec[1], all `DOMExceptions` should also be `Error`s (and therefore potentially have a stacktrace), though this isn't universally followed. In the cases in which it is, we should be capturing that stack.

Fixes #4085
Fixes #3119

[1] https://webidl.spec.whatwg.org/#es-DOMException-specialness
@nowylie
Copy link
Author

nowylie commented Nov 16, 2021

@AbhiPrasad @lobsterkatie no worries, thanks for merging a a fix! I look forward to having stack traces on these errors in the future :)

@AbhiPrasad
Copy link
Member

This is released with SDK v6.15.0: https://github.com/getsentry/sentry-javascript/releases/tag/6.15.0. Thanks again for the help @nowylie!

onurtemizkan pushed a commit that referenced this pull request Dec 19, 2021
Inspired by #4156 - h/t @nowylie.

According to the spec[1], all `DOMExceptions` should also be `Error`s (and therefore potentially have a stacktrace), though this isn't universally followed. In the cases in which it is, we should be capturing that stack.

Fixes #4085
Fixes #3119

[1] https://webidl.spec.whatwg.org/#es-DOMException-specialness
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.

Stack trace lost when capturing DOMException The stack trace of a SecurityError is not reported
3 participants