-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
Deprecate renderIntoDocument() #116
Comments
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 |
I think having a Other than that, we can't really trust For me, cleanup is part of the testing process. To make things more clear we could rename Another approach: test(() => {
const { destroy } = render()
// ...
destroy()
}) |
You make good points. And honestly, many people have a testSetup file where they they could do this repetitive work and reduce that friction. |
Yeah, we could import something like |
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? |
The only problem I see is that then importing the library would cause a side effect, which is a bad thing. |
We could change a bit the 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
So we would have:
|
Thanks for the ideas! I'm not sure that makes things any simpler. I feel like it would be a step backward.
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 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? |
For me the best approach would be suggesting the users to import Also if they call 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. |
I agree with what you said! Though
How do you propose we do that? |
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? |
Unfortunately not. It's totally valid to call render multiple times before a cleanup |
Yes, but |
Ahh... My code is wrong, what I really want is that importing |
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 |
That's not a problem though. We just need to register an |
I'm thinking that what I want to do is start off without have a |
@kentcdodds sounds like a good plan. |
Closes #116 BREAKING CHANGE: `renderIntoDocument` replaces `render` and `Simulate` is no longer exported (use `fireEvent` instead).
Closes #116 BREAKING CHANGE: `renderIntoDocument` replaces `render` and `Simulate` is no longer exported (use `fireEvent` instead).
Closes #116 BREAKING CHANGE: `renderIntoDocument` replaces `render` and `Simulate` is no longer exported (use `fireEvent` instead).
) Closes #116 BREAKING CHANGE: `renderIntoDocument` replaces `render` and `Simulate` is no longer exported (use `fireEvent` instead).
🎉 This issue has been resolved in version 4.0.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
The fundamental (and great) aspect of this library is:
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 moverenderIntoDocument()
's behavior torender()
.I don't see any reason to use
render()
overrenderIntoDocument()
.The text was updated successfully, but these errors were encountered: