Add strict check for dom type

[mbelsky]

Hey,

I face an issue and come with this pr :)

What:

Add strict check for dom's outerHTML field

Why:

To improve DX.

As a new user I've called debug(x) where x was a result of queryAll*. I've missed that queryAll* returns an array and got TypeError: Cannot read property 'length' of undefined in my terminal.

Checklist:

• Documentation added to the
  docs site N/A
• I've prepared a PR for types
• Tests
• Ready to be merged

[eps1lon]

I suppose the error is thrown at dom.outerHTML.length which means Element is sufficient. Otherwise we would also throw on svg elements.

Do we need to consider cross-realm instanceof checks here as well? Getting the window of an element would not work if we didn't get an Element in the first place. I think checking for typeof dom.outerHTML === 'string' might be sufficient here in asserting that the given value implements Element.

[mbelsky]

I suppose the error is thrown at dom.outerHTML.length which means Element is sufficient. Otherwise we would also throw on svg elements.

Unfortunately it isn't because document isn't an instance of Element

I think checking for typeof dom.outerHTML === 'string' might be sufficient here in asserting that the given value implements Element.

I absolutely agree that typeof dom.outerHTML === 'string' checking is enough however it may hide a real problem from a developer

[eps1lon]

Unfortunately it isn't because document isn't an instance of Element

Which is why you would keep the HTMLDocument check.

Cross-realm problem still exists.

I absolutely agree that typeof dom.outerHTML === 'string' checking is enough however it may hide a real problem from a developer

That would be?

IMO runtime type checking is not necessary here. If people care about type checking then you already have very good solutions with flow or typescript.

[mbelsky]

Types may lies. Right now dom typed as HTMLElement

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/403ac51161e5773d0ec7eb965004e18e9d4a03c8/types/testing-library__dom/pretty-dom.d.ts#L3

However there is a test for rendering document

dom-testing-library/src/__tests__/pretty-dom.js

Lines 39 to 40 in a401bb5

	test('prettyDOM supports receiving the document element', () => {
	expect(prettyDOM(document)).toMatchInlineSnapshot(`

Runtime checking isn't a silver bullet however it allows to fail fast when a library's user use it as not expected and teaches she how to do it right.

[eps1lon]

Types may lies

Did it occur to you that it was a mistake instead of an intention to specify something wrong?

[mbelsky]

Sure, it did.

For me as a developer it's so cool to have strict contracts in a library. What do you think about an alternative conditional here like:

- if (!(dom instanceof HTMLDocument) && !(dom instanceof HTMLElement)) {
-     throw new TypeError(
-      'Debuggable value should be an instance of HTMLDocument or HTMLElement.',
-    )
- }
+ if (!('outerHTML' in dom)) {
+     throw new TypeError(
+      'Debuggable object should have `outerHTML` field like an HTMLElement instance.',
+    )
+ }

[eps1lon]

Sounds better but you're still using HTMLElement which is too narrow. No need throw so many technicallaties around "Expected an element or document but got X".

[mbelsky]

Thanks for suggestion. I've updated this check and add a test

[mbelsky]

I didn't extend pretty-dom.js functional and cover added condition with a test. However ci fails with:

Jest: "global" coverage threshold for branches (100%) not met: 99.59%
...
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! @testing-library/dom@0.0.0-semantically-released validate: `kcd-scripts validate`
npm ERR! Exit status 1
npm ERR! 

Does it mean that I should write some tests for pretty-dom.js?

[eps1lon]

This branch is likely not covered. Also: This will throw on null because typeof null === 'object'.

[mbelsky]

Nice catch. Thank you for explaining.

[mbelsky]

inCypress has failed with null parameter so in addition I replaced arguments default initializations.

[kentcdodds]

This looks good to me 👍

[kentcdodds]

@all-contributors please add @mbelsky for code and tests

[allcontributors]

@kentcdodds

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

[kentcdodds]

🎉 This PR is included in version 6.6.0 🎉

The release is available on:

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

Your semantic-release bot 📦🚀