fix: Ignore comment nodes in toBeEmptyDOMElement

[marcelbarner]

What:
Adding a logic to the existing toBeEmptyDOMElement to ignore comments in the element.

Why:
In some frameworks, like Angular, there is a logic to create elements on specific conditions. If this condition is not solved than the framework also adds some comments, this comment has the effect that the matcher toBeEmptyDOMElement fails, but the element is empty from the perspective of the user.

How:
The matcher checks if the element contains child elements excluded comment nodes.

Checklist:

• Documentation
• Tests
• Updated Type Definitions
• Ready to be merged
  

[codecov]

Codecov Report

Merging #317 (559bc43) into master (f930668) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #317   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           23        23           
  Lines          534       537    +3     
  Branches       197       197           
=========================================
+ Hits           534       537    +3     

Impacted Files	Coverage Δ
src/to-be-empty-dom-element.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f930668...559bc43. Read the comment docs.

[nickserv]

It sounds like a good idea to support comments, but I think it would be easier to just patch toBeEmpty. I don't think there needs to be a distinction between empty elements and elements with only comments, as either way the element has no visible content.

[marcelbarner]

It sounds like a good idea to support comments, but I think it would be easier to just patch toBeEmpty. I don't think there needs to be a distinction between empty elements and elements with only comments, as either way the element has no visible content.

I am agreed with your point, but than it is not a empty element. An alternative solution will be to combine both logics to a new matcher with the name toContainNoVisibleContent or something similar.

[nickserv]

It may not technically be empty, but practically speaking there's nothing that would be displayed to the user, and it's better for tests to represent the user's perspective than implementation details.

[marcelbarner]

If we look from the user perspective, than I completely agree with your points. I will make the required adjustments.

[marcelbarner]

@nickmccurdy should I also add this logic to the matcher toBeEmpty? This matcher is marked as deprecated.
Also I have the problem to write a good test to validate that multiline comments are ignored. Can you maybe help me?

[gnapse]

I had some questions about the regex being used, but then it hit me that I have objections with using regex in the first place. It will most likely be error prone to use a regex approach for this purpose.

I'd suggest we can use the DOM structure itself, instead of parsing anything ourselves. We can traverse the list of nodes inside the target element, and check if it has any child nodes that are not comments.

Here's a snippet that may serve as inspiration:

const nonCommentChildNodes = [...element.childNodes].filter(
    node => node.nodeType !== Node.COMMENT_NODE
)

If this list turns out empty, the target element is empty, and vice-versa.

Would love your thoughts on this one.

[gnapse]

About this

should I also add this logic to the matcher toBeEmpty? This matcher is marked as deprecated.

I would not bother adding this fix to the deprecated matcher.

Also I have the problem to write a good test to validate that multiline comments are ignored. Can you maybe help me?

Using the DOM structure instead of parsing it ourselves will solve this.

[marcelbarner]

I had some questions about the regex being used, but then it hit me that I have objections with using regex in the first place. It will most likely be error prone to use a regex approach for this purpose.

I'd suggest we can use the DOM structure itself, instead of parsing anything ourselves. We can traverse the list of nodes inside the target element, and check if it has any child nodes that are not comments.

Here's a snippet that may serve as inspiration:

const nonCommentChildNodes = [...element.childNodes].filter(
    node => node.nodeType !== Node.COMMENT_NODE
)

If this list turns out empty, the target element is empty, and vice-versa.

Would love your thoughts on this one.

I think this idea is the better solution to use the DOM structure with that the code will be more stable. I had some concerns about node elements which only contain whitespaces or text, but I tried it and seems to work. I will adjust my implementation.

Thanks for your nice catch.

[nickserv]

My mistake, I forgot about the deprecation, I meant toBeEmptyDOMElement.

[marcelbarner]

No problem I am thank full for all of your great input, so thanks to you all.

[marcelbarner]

Now I need your help again, because I am not an expert for jsdom.
If I use this this snipped to identify that an node element is a COMMENT_NODE

const nonCommentChildNodes = [...element.childNodes].filter(node => node.nodeType !== Node.COMMENT_NODE);

I get the following error:

• ReferenceError: Node is not defined

After a research I found the following issue. I can not exactly identify what i have to do. I figured out that a workaround is to replace Node.COMMENT_NODE with 8 will work but I am not happy with a magic number or to redefine a global definition. So maybe you have an idea what i can do?

[gnapse]

I suggest you declare a constant of our own and add a jsdoc comment to it explaining why we can't use Node.COMMENT_NODE (including a link to that github issue).

[nickserv]

Aren't we targeting the JSDOM environment of Jest which includes these values in the V8 context? They're not really globals, but any code in Jest DOM should be able to pretend they're globals as far as I understand.

[gnapse]

Looking good. Requesting changes because of a few relatively minor observations. Let me know what you think.

[gnapse]

@all-contributors please add @marcelbarner for code and test

[allcontributors]

@gnapse

I've put up a pull request to add @marcelbarner! 🎉

[github-actions]

🎉 This PR is included in version 5.11.9 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[gnapse]

@all-contributors please add @marcelbarner for code and test

[allcontributors]

@gnapse

I've put up a pull request to add @marcelbarner! 🎉