Skip to content

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

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

Closed
wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bcec344
feat(pretty-format): Enable filtering of children in DOMElement
eps1lon Feb 28, 2021
bb749e9
feat(pretty-format): non-text nodes serialized to empty strings are i…
eps1lon Feb 28, 2021
54d5bac
be explicit about text node
eps1lon Mar 1, 2021
31576fc
Add changelog entry
eps1lon Mar 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
- `[jest-transform]` Pass config options defined in Jest's config to transformer's `process` and `getCacheKey` functions ([#10926](https://github.com/facebook/jest/pull/10926))
- `[jest-worker]` Add support for custom task queues and adds a `PriorityQueue` implementation. ([#10921](https://github.com/facebook/jest/pull/10921))
- `[jest-worker]` Add in-order scheduling policy to jest worker ([10902](https://github.com/facebook/jest/pull/10902))
- `[pretty-format]` Ignore DOM nodes (except text nodes) that are serialized as an empty string (only affects usages of `pretty-format` with custom plugins). ([11130](https://github.com/facebook/jest/pull/11130))

### Fixes

Expand Down
47 changes: 47 additions & 0 deletions packages/pretty-format/src/__tests__/DOMElement.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,3 +583,50 @@ Testing.`;
);
});
});

describe('DOMElement Plugin with a null printer', () => {
beforeAll(() => {
setPrettyPrint([
{
serialize() {
return '';
},
test(value) {
return (
DOMElement.test(value) &&
(value.tagName === 'SCRIPT' || value.nodeType === 8)
);
},
},
DOMElement,
]);
});

afterAll(() => {
setPrettyPrint([DOMElement]);
});

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

div.innerHTML = `...`

const script = document.createElement('script');
script.setAttribute('src', 'index.js');
div.appendChild(script);

const comment = document.createComment('Hello, Dave!');
div.appendChild(comment);

const main = document.createElement('main');
main.appendChild(document.createTextNode('Hello, World!'));
div.appendChild(main);

const expected = [
'<div>',
' <main>',
' Hello, World!',
' </main>',
'</div>',
].join('\n');
expect(div).toPrettyPrintTo(expected);
});
});
26 changes: 19 additions & 7 deletions packages/pretty-format/src/plugins/lib/markup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ export const printProps = (
.join('');
};

// https://developer.mozilla.org/en-US/docs/Web/API/Node/nodeType#node_type_constants
const NodeTypeTextNode = 3;

// Return empty string if children is empty.
export const printChildren = (
children: Array<unknown>,
Expand All @@ -62,14 +65,23 @@ export const printChildren = (
printer: Printer,
): string =>
children
.map(
child =>
config.spacingOuter +
indentation +
(typeof child === 'string'
.map(child => {
const printedChild =
typeof child === 'string'
? printText(child, config)
: printer(child, config, indentation, depth, refs)),
)
: printer(child, config, indentation, depth, refs);

if (
printedChild === '' &&
typeof child === 'object' &&
child !== null &&
(child as Node).nodeType !== NodeTypeTextNode
) {
// A plugin serialized this Node to '' meaning we should ignore it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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. :/

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

return '';
}
return config.spacingOuter + indentation + printedChild;
})
.join('');

export const printText = (text: string, config: Config): string => {
Expand Down