Deprecate toBeInTheDOM in favor of toContainElement

[gnapse]

Describe the feature you'd like:

It's been previously noted that toBeInTheDOM does not really perform the check that its name suggests (testing-library/dom-testing-library#9, #3). This was partially addressed in #25, where also toContainElement was introduced.

However, toBeInTheDOM continues to feel not right, at least for me. It now has two different behaviors: if it receives an element as argument, it'll check that the expect argument is contained within that extra element. If it does not receive an extra element as argument, it behaves as a glorified .toBeInstanceOf(HTMLElement), merely checking that the received expect argument is a DOM element, regardless of whether is contained anywhere.

Suggested implementation:

I propose that we remove toBeInTheDOM, maybe with a deprecation period.

Describe alternatives you've considered:

At the very least, we should update the documentation in the README about it. This current intro to this matcher is not accurate about what it does:

This allows you to assert whether an element present in the DOM container or not. If no DOM container is specified it will use the default DOM context.

There's no default DOM context (whatever that means). This matcher's only task when no DOM container is specified, is to assert that the given element is indeed an HTML (or SVG since recently) element.

Teachability, Documentation, Adoption, Migration Strategy:

We would need to update the documentation and provide users with alternatives moving forward:

One recommendation is to replace current uses with toContainElement.

Also, if users just want to check if an element exist, they can assert that what they have is not null or undefined. For instance, using dom-testing-library queries and regular jest matchers:

expect(queryByTestId('ok-button')).toBeTruthy();
expect(queryByTestId('ok-button')).toBeInstanceOf(HTMLElement);

Also, this would be a breaking change, so a major version bump is involved and users would have to explicitly update to get the change and be really forced to replace its uses.

[kentcdodds]

I'm in favor 👍

[smacpherson64]

Same! What was the original use case of toBeInTheDOM? Was it for Portals?

toBeInTheDOM requires a DOM context. This may be my misunderstanding of jsdom but instead of checking if an object instance is a SVGElement or HTMLElement, could we use document.documentElement instead? It would be the same as doing the following:

expect(document.documentElement).toContainElement(element);

If we do end up keeping toBeInTheDOM, I think you are right @gnapse the optional container makes it a bit confusing and should be removed.

Thoughts @gnapse ?

[gnapse]

Good point. If we can make expect(element).toBeInTheDOM() to be equivalent to

expect(document.documentElement).toContainElement(element)

Then it would make more sense. And in that case we wouldn't need to deprecate it, because it does read nicely. Not sure though about if this breaks the current functionality.

Would love the input of others.

[kentcdodds]

I think I'm in favor of that, but what if instead we called it toBeInTheDocument instead? "The DOM" is a little ambiguous in a testing situation...

Also, I would be wary of relying on this a lot because in the react-testing-library circumstance, once we can stop rendering things to the document (when this is fixed) then we will (it will be a breaking change). So anyone using this method will suddenly have all those tests failing and they'll have to migrate them (which is probably fine, but would be kinda annoying). Anyway, I think making it have a more explicit name would probably be a good thing.

[smacpherson64]

Hrm, if it will eventually break I think we should remove it sooner rather than later :)! @kentcdodds will we then lose the ability to use document? Like how toHaveFocus works now?

[jgoz]

I like the idea of renaming it to toBeInTheDocument and asserting that the element is somewhere in the document, even if react-testing-library stops rendering into the document by default. This would be a useful assertion on its own, independent of how react-testing-library works. Portals, non-react modals, etc.

[gnapse]

Like how toHaveFocus works now?

to be clear, what @kentcdodds mentioned affects react-testing-library, not this library

And now that I think about it (maybe it's an issue to raise in react-testing-library) but is "not rendering to the document" something that really reflects how the final user uses the app? 🤔 (I'm thinking of the guiding principle here).

[smacpherson64]

Really good points! I often forget to scope jest-dom and react-testing-library separately but they are distinct. :)!

For the name change either way is fine.

• One plus for toBeInTheDOM is it is already being used, so it is likely familiar.

• One plus for toBeInTheDocument is toBeInTheDOM has changed implementations and is known to not accomplish what it’s name suggests.

What is the difference between the DOM and the document in this context?

[gnapse]

What is the difference between the DOM and the document in this context?

Not sure. toBeInTheDOM was never too faithful to what it really does (which is the main reason for this proposal). I'd say that if we do this, we make the name change. Or more accurately, is not a name change, it's the introduction of a new matcher (toBeInTheDocument) and the removal of an existing matcher (toBeInTheDOM) because the former does not replace the latter exactly.

I think we can go forward with providing the new one first, and then we can remove the other one in a separate release. We can even add some note to the documentation about toBeInTheDOM being deprecated, and maybe even some deprecation warning as well.

I favor the new behavior with the new name, because it's more close the name to what it does. As your very question above implies, the name toBeInTheDOM is not very clear about what "the DOM" is.

[kentcdodds]

Let's do it!

[smacpherson64]

I don’t mind attacking this one. I should be able to have it done by the end of day on Monday.

For the depreciation, along with putting it in the readme is it ok to put a warning
on the actual function call?

console.warn('toBeInTheDOM matcher has been depreciated and will be removed in future updates. Please use toBeInTheDocument instead.')

[smacpherson64]

Implemented in #40