Skip to content

Use defineProperty on the error object instead of setting the message… #1261

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Sep 17, 2023
Merged

Use defineProperty on the error object instead of setting the message… #1261

merged 2 commits into from
Sep 17, 2023

Conversation

@julienw
Copy link
Contributor

@julienw julienw commented Sep 15, 2023 •
edited
Loading

… directly

Fixes #1259

What: Use defineProperty on the error object instead of setting the message directly (Fixes #1259)

Why: With a DOMException, the error isn't propagated properly without this fix.

How: I used defineProperty instead of setting the message property directly.

Checklist:

  • Documentation added to the docs site unneeded
  • Tests
  • TypeScript definitions updated unneeded
  • Ready to be merged
timdeschryver
timdeschryver approved these changes Sep 15, 2023
View reviewed changes
Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

This looks good to me.
It's not breaking, but we can take add this to the aplha branch and release it with the next major version.
@julienw
Copy link
Contributor Author

julienw commented Sep 16, 2023

Thanks for the review!
If it's not breaking, why isn't it possible to take it in a minor version ?
eps1lon
eps1lon approved these changes Sep 17, 2023
View reviewed changes
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Thanks!
@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 17, 2023

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 a0e916f:

Sandbox Source
react-testing-library-examples Configuration
@codecov
Copy link

codecov bot commented Sep 17, 2023 •
edited
Loading

Codecov Report

Merging #1261 (a0e916f) into main (07f5999) will not change coverage.
The diff coverage is 100.00%.

@@ Coverage Diff @@ ## main #1261 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 24 24 Lines 1038 1038 Branches 351 351 ========================================= Hits 1038 1038
Flag Coverage Δ
node-14 100.00% <100.00%> (ø)
node-16 100.00% <100.00%> (ø)
node-18 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/wait-for.js 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more
@timdeschryver
Copy link
Member

timdeschryver commented Sep 17, 2023

Nevermind, scratch my previous comment @julienw .
There was a workaround around our CI issue, which is resolved in #1262
@timdeschryver timdeschryver merged commit 270a531 into testing-library:main Sep 17, 2023
@timdeschryver
Copy link
Member

timdeschryver commented Sep 17, 2023

@all-contributors please add @julienw for code, bug
@allcontributors allcontributors bot mentioned this pull request Sep 17, 2023
Merged
@allcontributors
Copy link
Contributor

allcontributors bot commented Sep 17, 2023

@timdeschryver

I've put up a pull request to add @julienw! 🎉
eps1lon pushed a commit to eps1lon/dom-testing-library that referenced this pull request Oct 3, 2023
@julienw
fix: use defineProperty on the error object instead of setting the me…
e6ba7e0 
…ssage directly (testing-library#1261) Fixes testing-library#1259
@eps1lon eps1lon mentioned this pull request Oct 3, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants

@julienw @timdeschryver @eps1lon