Skip to content

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

Closed
nowylie wants to merge 1 commit into
getsentry:masterfrom
nowylie:fix-dom-exception
Closed

fix(browser): Don't special case DOMExceptions that are also Errors#4156
nowylie wants to merge 1 commit into
getsentry:masterfrom
nowylie:fix-dom-exception

Conversation

@nowylie
Copy link
Copy Markdown

@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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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