Deprecate renderIntoDocument() about react-testing-library HOT 19 CLOSED

I'm thinking that what I want to do is start off without have a react-testing-library/cleanup file and just do as simple as possible. Remove Simulate, replace render with renderIntoDocument, and publish a breaking change. I think I'll work on this myself.

from react-testing-library.

🎉 This issue has been resolved in version 4.0.0 🎉

The release is available on:

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

Your semantic-release bot 📦🚀

from react-testing-library.

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.

from react-testing-library.

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()
})

from react-testing-library.

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

from react-testing-library.

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

from react-testing-library.

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?

from react-testing-library.

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

from react-testing-library.

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 }])

from react-testing-library.

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?

from react-testing-library.

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.

from react-testing-library.

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?

from react-testing-library.

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?

from react-testing-library.

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

from react-testing-library.

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

from react-testing-library.

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

from react-testing-library.

What about:

// index.js

let willCleanup = false

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

// or setupCleanup, I don't know the best name
function configureCleanup(beforeEach) {
  if (beforeEach !== undefined) {
    beforeEach(cleanup)
    willCleanup = true
  }
}

export { render, configureCleanup, ... }

// cleanup.js

import { configureCleanup } from "./"

configureCleanup(beforeEach)

If beforeEach is not available, the user can import configureCleanup himself (instead of react-testing-library/cleanup) and pass the correct beforeEach fn.

from react-testing-library.

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

That's not a problem though. We just need to register an afterEach(cleanup) which shouldn't be a problem at all.

from react-testing-library.

@kentcdodds sounds like a good plan.

from react-testing-library.