fix: check for any Node in toHaveTextContent #307
Conversation
nickmccurdy
changed the title
nickmccurdy
force-pushed
the
check-for-any-node
branch
from
25089e3
to
32cdb02
Compare
nickmccurdy
changed the title
nickmccurdy
changed the title
nickmccurdy
force-pushed
the
check-for-any-node
branch
from
32cdb02
to
9af9394
Compare
nickmccurdy
changed the title
|
This needs more work on the error handling. |
|
You're right. The error message is fixed, just need to fix the rest and add the error message to tests. |
|
#312 reveals that this may not be the only custom matcher that has to allow any node and not HTML elements. It may be worth checking all our matchers to see which other ones make sense to have this change done. Although a quick look at our list of custom matchers makes me think these may be the only two exceptions, but I may be wrong. |
|
That's an issue with DOM Testing Library, not Jest DOM. It makes sense to update all other matchers to use |
gnapse
force-pushed
the
check-for-any-node
branch
from
9af9394
to
d6dd1b7
Compare
There was a problem hiding this comment.
@nickmccurdy I rebased this and tried to work out the CI build failure. It is due to there not being a test that covers the failure case (a test that explicitly makes it fail when it receives something other than a Node).
I then tried to do fix it myself but it became non-trivial. See my comment below. I then decided to leave it to you to decide how to approach this, as you are the PR author. Let me know and can take it from here if you are not able to deal with this in the short term.
|
|
||
| export function toHaveTextContent( | ||
| htmlElement, | ||
| checkWith, | ||
| options = {normalizeWhitespace: true}, | ||
| ) { | ||
| checkHtmlElement(htmlElement, toHaveTextContent, this) | ||
| const window = htmlElement.ownerDocument.defaultView |
There was a problem hiding this comment.
There should be a check prior to this line. Otherwise this line can throw when receiving something that does not even have a ownerDocument property. Or if the obtained window value is not a window object and does not have a window.Node thing in it.
|
hey @nickmccurdy, any luck with moving this forward? :-) Needs a hand? |
|
Superseded by #358 Anyway, thanks @nickmccurdy for giving it a shot here. |
What:
Fixes #306
How:
!(htmlElement instanceof Node)Checklist: