feat(pretty-format): Enable filtering of children in DOMElement #11130

eps1lon · 2021-02-28T18:27:28Z

Summary

Allow ignoring "noise" in the DOM in pretty-format

Given

<body>
  <script
    src="context.js"
  />
  <!-- Some comment -->
  <p>
    Hello, Dave
  </p>
</body>

I want to be able to pretty-format only the "relevant" parts:

<body>
  <p>
    Hello, Dave
  </p>
</body>

"relevant" means the parts that the tested code controls. Some testing frameworks (e.g. mocha or karma) insert script tags and comments which take up considerable space.

Test plan

CI green
use feature to close Allow narrowing down the part of the DOM that is printed when queries fail testing-library/dom-testing-library#902

…gnored

packages/pretty-format/src/plugins/lib/markup.ts

SimenB

changelog entry? 🙂

This makes sense to me, but possibly something that should be mentioned in the readme? Dropping empty strings might be unexpected, dunno?

eps1lon · 2021-03-01T10:27:36Z

changelog entry? slightly_smiling_face

Will do.

Dropping empty strings might be unexpected, dunno?

This would only change existing behavior if people had custom plugins that handle an Element or CommentNode.

SimenB · 2021-03-01T11:03:41Z

Dropping empty strings might be unexpected, dunno?

This would only change existing behavior if people had custom plugins that handle an Element or CommentNode.

right! that seems somewhat unlikely

jeysal · 2021-03-02T19:09:45Z

packages/pretty-format/src/plugins/lib/markup.ts

+        child !== null &&
+        (child as Node).nodeType !== NodeTypeTextNode
+      ) {
+        // A plugin serialized this Node to '' meaning we should ignore it.


Hmm, I'm not so excited about taking away the ability of plugins to output empty text for a node, esp. in a breaking way. Would it be an option to allow returning null instead?

Hmm, I'm not so excited about taking away the ability of plugins to output empty text for a node, esp. in a breaking way.

My question would be: Why is that desired? Why write a plugin that takes a Node, serializes it to an empty string but also expect that empty string to be included in the snapshot.

Would it be an option to allow returning null instead?

I've tried this route but it propagated the possibly null return all the way up so that other packages started to break.
I'll check again if we can change the boundaries but so far it seemed there are too many places that are accidentally coupled.

I'm not saying I have a concrete use case, but I know how many usages are out there pretty-printing element trees that are abstractions over HTML (if you need a horrifying example, take a look at 'ExtReact'). If in just one of them one renders plain text not with string children, but with createElem('Label', {text: ''}), and their pretty printer converts the custom component language to the actual HTML, it will now no longer print the right representation of the element tree. With something used in as many wildly different tech stacks as Jest is, it is bound to break someone.
I would follow principle of least surprise — if an actual '' text child is printed as an empty (except indented) line, you would expect a child that is mapped to '' by the printer to behave the same. I don't know the empty string to typically signalize omission in other contexts either.

Regarding an explicit omission return value, we would want that limited specifically to printChildren, because while printing children you can omit one, not in all other contexts.
I would expect that changing the type of printChildrens printer argument to something like the following would work?

type NullablePrinter = (...args: Parameters<Printer>) => ReturnType<Printer> | null

Actually, I've just looked at the way you implemented your plugin, and this would not really help you because you are not in control of the printChildren call, instead you rely on the standard DOMElement or other plugin to make that call, passing the standard printer along which will then delegate to your plugin. :/

I believe we have both been thinking about this the wrong way.

Printing

<div> ${print('asdf')} ${print('')} </div>

and expecting the print('') to be able to remove the whole third line is like printing

Object { a: ${print(42)} b: ${print(null)} }

and expecting the print(null) to be able to remove the whole third line for a toEqualIgnoringNulls() matcher.

print('') at top level should have the same result as print('') in an element child, and print('') at top level does not return a zero-line string either (because a zero-line string is not a thing, it would have -1 line breaks in it).

I think what should really be done to omit children from the output is not pass them into a printer in the first place. Replace the DOMElement plugin with a plugin under your control, that clones the node, throws out any children you don't want, then pass it to the DOMElement plugin to reuse its code. Let me know if this works for you.

Actually, I've just looked at the way you implemented your plugin, and this would not really help you because you are not in control of the

Did you see the test added?

Yes, the sentence you quoted referred to the previous comment, not to the implementation in this PR.

I don't understand why you can say the current implementation doesn't help me when the test that uses the new implementation is exactly my desired outcome.

Again, I am not saying what you implemented here does not work for you, I am aware it does. The second of the three comments says that what I suggested in the second half of the first comment actually would not work for you after I reconsidered it.
For why this change will lead to surprising behavior, see the first half of my first comment.
For an alternative solution that I believe would work without introducing this surprising behavior, see my third comment.

jeysal · 2021-03-02T19:11:45Z

packages/pretty-format/src/__tests__/DOMElement.test.ts

+
+  it('filters children', () => {
+    const div = document.createElement('div');
+


If possible I think it would be easier to compare this to the expected if this was being constructed via

div.innerHTML = `...`

eps1lon · 2021-04-25T10:01:45Z

We're just going to fork the pretty-format dom plugin. Cloning DOM trees is not viable and it seems like you intend to keep printing empty strings on a new line.

github-actions · 2021-05-26T00:09:25Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

eps1lon added 2 commits February 28, 2021 18:06

feat(pretty-format): Enable filtering of children in DOMElement

bcec344

feat(pretty-format): non-text nodes serialized to empty strings are i…

bb749e9

…gnored

facebook-github-bot added the cla signed label Feb 28, 2021

eps1lon commented Feb 28, 2021

View reviewed changes

packages/pretty-format/src/plugins/lib/markup.ts Outdated Show resolved Hide resolved

SimenB reviewed Feb 28, 2021

View reviewed changes

SimenB requested a review from jeysal February 28, 2021 21:55

eps1lon added 2 commits March 1, 2021 11:31

be explicit about text node

54d5bac

Add changelog entry

31576fc

eps1lon added a commit to eps1lon/dom-testing-library that referenced this pull request Mar 1, 2021

REVERT LATER leverage jestjs/jest#11130

4295727

eps1lon mentioned this pull request Mar 1, 2021

feat: Add filterNode option to prettyDOM testing-library/dom-testing-library#907

Merged

4 tasks

jeysal reviewed Mar 2, 2021

View reviewed changes

eps1lon closed this Apr 25, 2021

eps1lon deleted the feat/pretty-form/FilterDOMElement branch April 25, 2021 10:01

github-actions bot locked as resolved and limited conversation to collaborators May 26, 2021


		it('filters children', () => {
		const div = document.createElement('div');

Uh oh!

feat(pretty-format): Enable filtering of children in DOMElement #11130

feat(pretty-format): Enable filtering of children in DOMElement #11130

Uh oh!

Conversation

eps1lon commented Feb 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

Uh oh!

SimenB left a comment

Choose a reason for hiding this comment

Uh oh!

eps1lon commented Mar 1, 2021

Uh oh!

SimenB commented Mar 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eps1lon commented Apr 25, 2021

Uh oh!

github-actions bot commented May 26, 2021

Uh oh!

Uh oh!

eps1lon commented Feb 28, 2021 •

edited

Loading