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

feat: Reduce caught exceptions in prettyDom #1321

Merged
merged 2 commits into from
Jul 2, 2024

Conversation

DavidRieman
Copy link
Contributor

@DavidRieman DavidRieman commented Jun 25, 2024

What:
Added tests for pretty-dom to guard against potential behavioral regressions.
Then optimized shouldHighlight in pretty-dom to avoid unexceptional exceptions.

Why:
When developers are using the debugging features to halt the debugger upon "caught exceptions" or "first-chance exceptions" as it may be called in some IDEs, unrelated test failures were disruptively stealing the developer's attention and contributing to premature test timeouts. These disruptions are generally unnecessary since the situation leading up to it can be easily avoided with refactoring.

The exception at hand is that JSON.parse throws when passed undefined and/or empty string values. It can be easily avoided because we know that it will throw, and we know the intent is to default the colorization selection.

How:
Streamlined the logic in shouldHighlight to preserve the final result while including extra safety checks to avoid passing undefined/empty to JSON.parse.

Checklist:

  • N/A Documentation added to the docs site N/A
  • Tests
  • N/A TypeScript definitions updated N/A
  • Ready to be merged

…ghlight in pretty-dom. (These were often disruptive to developers who need to catch some other first-chance exception in the act as they may have to wade through these unexceptional occurrences on the way to get to their target first-chance exception.)
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit f7ccda2:

Sandbox Source
react-testing-library-examples Configuration

@eps1lon
Copy link
Member

eps1lon commented Jun 25, 2024

Basically we reduce the number of caught exceptions?

@DavidRieman
Copy link
Contributor Author

DavidRieman commented Jul 1, 2024

Indeed, unexceptional exceptions are a thorny problem in the JS ecosystem due to continued lack of sufficient IDE support for the kind of "just my code" debugger halting features that have existed for decades in other ecosystems. (This one in particular stings me at least once a year so I finally resolved to fix it at the source.) What are our next steps?

@eps1lon eps1lon changed the title Fix pretty dom exception feat: Reduce caught exceptions in prettyDom Jul 2, 2024
@eps1lon eps1lon merged commit 76cb73d into testing-library:main Jul 2, 2024
10 checks passed
} else {
// If `colors` is not set, colorize if we're in node.
return (
typeof process !== 'undefined' &&
Copy link
Member

Choose a reason for hiding this comment

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

This always needs to stay for environments that don't have process e.g. browsers: #1323

eps1lon added a commit to eps1lon/dom-testing-library that referenced this pull request Jul 5, 2024
@eps1lon
Copy link
Member

eps1lon commented Jul 5, 2024

Needs to be reverted in #1325 since it caused #1322. Relanding in #1323

eps1lon added a commit that referenced this pull request Jul 5, 2024
eps1lon pushed a commit that referenced this pull request Jul 5, 2024
Co-authored-by: David Rieman <david.rieman@wbd.com>
eps1lon pushed a commit that referenced this pull request Jul 5, 2024
Co-authored-by: David Rieman <david.rieman@wbd.com>
eps1lon pushed a commit that referenced this pull request Jul 22, 2024
Co-authored-by: David Rieman <david.rieman@wbd.com>
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.

2 participants