Deprecate renderIntoDocument()

[hnordt]

The fundamental (and great) aspect of this library is:

The more your tests resemble the way your software is used, the more confidence they can give you.

I've watched all your talks and you keep saying that Simulate is a bad idea (even Dan Abramov told us that, if I'm not wrong) and I really agree with you!

We all want to make testing easier, and the only confusing thing that I see in this great library is having two render methods.

My suggestion is that we just deprecate renderIntoDocument() and move renderIntoDocument()'s behavior to render().

I don't see any reason to use render() over renderIntoDocument().

[kentcdodds]

Thanks for bringing this up. As soon as this is done then I will be willing to do that. Until then, we have to have the cleanup method and I think that would be annoying/confusing as well. I'm happy to hear other's thoughts on the matter though.

[hnordt]

We could encourage people to have a beforeEach that cleans things up, but that'd be annoying and reduce the simplicity.

I think having a beforeEach(cleanup) is a lot simpler than having both render(), renderIntoDocument() and Simulate. Also we need to explain when to use element.click() over Simulate.click() and explain why element.click() or element.focus() can't be used if render() is being used.

Other than that, we can't really trust Simulate, so the additional beforeEach(cleanup) effort would be fine as the trade-off would be increasing confidence.

For me, cleanup is part of the testing process. To make things more clear we could rename cleanup() to removeRenderedContainerFromDocument() or something like that.

Another approach:

test(() => {
  const { destroy } = render()
  // ...
  destroy()
})

[kentcdodds]

You make good points. And honestly, many people have a testSetup file where they they could do this repetitive work and reduce that friction.

[hnordt]

Yeah, we could import something like react-testing-library/auto-cleanup in setupTests.js.

[kentcdodds]

What if we did it automatically 🤔 Like put this at the top of our main file:

if (typeof beforeEach !== undefined) {
  beforeEach(cleanup)
}

Hmmmm..... Why would that be bad?

[hnordt]

The only problem I see is that then importing the library would cause a side effect, which is a bad thing.

[hnordt]

We could change a bit the render() usage:

test('sync test', () => {
  render(<Login />, ({ getByLabelText }) => {
    getByLabelText('username')
    getByLabelText('username').value = 'chucknorris'
  })
})

test('async test', () => {
  render(<Login />, async ({ getByLabelText }) => {
    await wait(() => getByLabelText('username'))
    getByLabelText('username').value = 'chucknorris'
  })
})

The implementation:

const render = async (..., callback) => {
  const publicAPI = ...
  await callback(publicAPI)
  
  document.body.removeChild(container)
  ReactDOM.unmountComponentAtNode(container)
}

The nice thing about this approach is that it matches the ReactDOM.render() API:

ReactDOM.render(element, container[, callback])

So we would have:

ReactTesting.render(element[, container], callback)

or

ReactTesting.render(element, callback[, options = { container, baseElement }])

[kentcdodds]

Thanks for the ideas! I'm not sure that makes things any simpler. I feel like it would be a step backward.

The only problem I see is that then importing the library would cause a side effect, which is a bad thing.

I can understand that perspective. The side-effect is basically unobservable though so I think it'd probably be ok, but I wont push it.

I think I'm in favor of just deprecating renderIntoDocument and putting its logic into render as you originally suggested. Then we'd just tell people to make sure to have a beforeEach(cleanup) configured. We could also expose a module as you suggested. So people could do:

import 'react-testing-library/cleanup'
import {render} from 'react-testing-library'

I don't think that's too much to ask.

We could also re-export everything from cleanup:

import {render} from 'react-testing-library/cleanup'

Thoughts?

[hnordt]

For me the best approach would be suggesting the users to import react-testing-library/cleanup on their global test setup script or import it on every test if they don't have a global test setup script.

Also if they call render() and we didn't detect that cleanup() was called we could throw an error.

import {render} from 'react-testing-library/cleanup'

I like the simplicity of it and I dislike that it's not very semantic. But I'm not opposed to it, I don't see any other problems than the semantic issue.

[kentcdodds]

I agree with what you said! Though

Also if they call render() and we didn't detect that cleanup() was called we could throw an error.

How do you propose we do that?

[hnordt]

Hum...

let willCleanup = false

function render() {
  if (!willCleanup) throw new Error(`You forgot to import "react-testing-library/cleanup"!`)
  // ...
}

function cleanup() {
  // ...
  willCleanup = true
}

It would work?

[kentcdodds]

Unfortunately not. It's totally valid to call render multiple times before a cleanup

[hnordt]

Yes, but import 'react-testing-library/cleanup' is a side effect, so it would run before the test itself.

[hnordt]

Ahh... My code is wrong, what I really want is that importing react-testing-library/cleanup sets willCleanup to true.